Skip to content

Conversation

@SteNicholas
Copy link
Member

@SteNicholas SteNicholas commented Mar 1, 2024

What changes were proposed in this pull request?

Bump ap-loader from 3.0(v8) to support for async-profiler 3.0.

Why are the changes needed?

ap-loader 3.0(v8) has already been released, which supports for async-profier 3.0. The release guide refers to Loader for 3.0 (v8): Binary launcher and AsyncGetCallTrace replacement. Breaking changes with async-profiler 3.0:
async-profiler 3.0 changed the meaning of the --lib option from --lib path full path to libasyncProfiler.so in the container to -l, --lib prepend library names, so the AsyncProfilerLoader will throw an UnsupportedOperation exception when using the --lib option with a path argument and async-profiler 3.0 or higher.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA. Tested in production environment of Spark cluster.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the BUILD label Mar 1, 2024
@SteNicholas
Copy link
Member Author

SteNicholas commented Mar 1, 2024

cc @dongjoon-hyun, @parthchandra, @mridulm.

@dongjoon-hyun
Copy link
Member

Thank you.

Copy link
Contributor

@parthchandra parthchandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-47242][CONNECT][PROFILER] Bump ap-loader 3.0(v8) to support for async-profiler 3.0 [SPARK-47242][BUILD] Bump ap-loader 3.0(v8) to support for async-profiler 3.0 Mar 1, 2024
@dongjoon-hyun
Copy link
Member

To @SteNicholas , for this kind of PR, we can use simply [BUILD] tag like GitHub Action bot suggested.

Screenshot 2024-03-01 at 11 39 13

To @parthchandra , could you confirm that this works without any issues, please? We don't have a test coverage in CI, do we?

@parthchandra
Copy link
Contributor

Yes there is no good way to test this in ci. Let me try it out and make sure.

@dongjoon-hyun
Copy link
Member

Thank you so much, @parthchandra !

@SteNicholas
Copy link
Member Author

@parthchandra, thank you try it out. Have you tried anything wrong?

@HyukjinKwon
Copy link
Member

The original PR does not have its test together (#44021). @SteNicholas Mind describing how you tested this?

@SteNicholas
Copy link
Member Author

The original PR does not have its test together (#44021). @SteNicholas Mind describing how you tested this?

@HyukjinKwon, I only tested this bump in production environment of Spark cluster.

@HyukjinKwon
Copy link
Member

That's fine. My point is that please describe it in the PR description.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

@parthchandra
Copy link
Contributor

@parthchandra, thank you try it out. Have you tried anything wrong?

I was able to try it out locally (non production) and the jfr files written were fine. I didn't see much difference in the output for my job, but others may definitely benefit.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @SteNicholas , @parthchandra , @HyukjinKwon .
Merged to master for Apache Spark 4.0.0.

@dongjoon-hyun
Copy link
Member

I added you to the Apache Spark contributor group, @SteNicholas , and assigned SPARK-47242 to you.
Welcome to the Apache Spark community!

ericm-db pushed a commit to ericm-db/spark that referenced this pull request Mar 5, 2024
…iler 3.0

### What changes were proposed in this pull request?

Bump ap-loader from 3.0(v8) to support for async-profiler 3.0.

### Why are the changes needed?

ap-loader 3.0(v8) has already been released, which supports for async-profier 3.0. The release guide refers to [Loader for 3.0 (v8): Binary launcher and AsyncGetCallTrace replacement](https://github.com/jvm-profiling-tools/ap-loader/releases/tag/3.0-8). Breaking changes with async-profiler 3.0:
async-profiler 3.0 changed the meaning of the `--lib` option from `--lib path        full path to libasyncProfiler.so in the container` to `-l, --lib         prepend library names`, so the `AsyncProfilerLoader` will throw an `UnsupportedOperation` exception when using the --lib option with a path argument and async-profiler 3.0 or higher.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass GA. Tested in production environment of Spark cluster.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#45351 from SteNicholas/SPARK-47242.

Authored-by: SteNicholas <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants