Set user agent when downloading jars with local_jar - #36342
Conversation
1be2aea to
4e0de77
Compare
4e0de77 to
77ada10
Compare
|
It's tricky to enter e2e tests (from pipeline option to check user agent in request) because the code path is excluded from dev Beam: needed to comment out these lines to make test script pass over this check. Tested that |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
| 'sdk worker container image.')) | ||
| parser.add_argument( | ||
| '--user_agent', | ||
| default=None, |
There was a problem hiding this comment.
can we set new default e.g. "Apache Beam" ?
There was a problem hiding this comment.
I agree - from conversations with the sonatype folks, it sounds like having a default would be very helpful
There was a problem hiding this comment.
The default is set here: https://github.com/apache/beam/pull/36342/files#diff-fd140abcfcbb9b91db28c94a0458d02b4eb21f7f61db4935bf501464b27e1cfeR291 if user not setting this value the user agent is set to "Apache Beam Python SDK/2.xx.0"
The problem of default pipeline options is that Java already has a --userAgent option defaults to "Apache Beam Java SDK/2.xx.0". This change is essentially port the option to Python. However Java side default is handled by a DefaultFactory class, but if I set a non-empty str as default in Python, I afraid am it could pass to Java side when running portable pipelines / xlang pipelines, or other unintended result
|
Assigning reviewers: R: @liferoad for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --user_agent option to specify a user agent when downloading JAR files. The changes are well-implemented, propagating the new option from the pipeline configuration down to the network request level. A default user agent is also thoughtfully included. I've found a minor but important typo in one of the files that needs to be corrected.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for setting a user agent when downloading JAR files. The changes correctly plumb the user_agent option from pipeline options down to the urlopen call. I have two main points of feedback regarding potential issues with object state and caching that could lead to unexpected behavior. Other than these points, the changes look good and effectively implement the desired feature.
|
MilvusException in Python3.12 ML PreCommit, not related to this change |
4 participants
Related: #36338
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.