-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass runtime configs & variables to BeamSqlSeekableTable #28253
Conversation
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
R: @Abacn |
R: @bvolpato |
R: @kennknowles |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Thanks! No major concerns from me -- just one note is that relying on PipelineOptions will allow configuring behavior for the entire job. Sometimes it's better to use a specific configuration class / instance, so you could likely have multiple JOINs in the same job and each handle different things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It's totally reasonable to add the method as they corresponding the dofn lifecycle methods. Probably we should add them at the beginning.
- One thing to note when implementing youe own BeamSqlSeekableTable. startBundle can be called many times, so if initializing a connection in it, it's good to use a connection pool otherwise there could be too much connection - a common issue seen in Beam IO implementation
also, its nice to have a test checking dofn lifecycle is effective with BeamSqlSeekableTable methods.
...va/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/BeamSqlSeekableTable.java
Outdated
Show resolved
Hide resolved
thanks very much. I will add an initialized flag to startBundle in our own BeamSqlSeekableTable to avoid too much connection. |
fixes #28145