Fix PostCommit Java Nexmark Dataflow job #33979
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #33979 +/- ##
=========================================
Coverage 59.08% 59.09%
- Complexity 3238 3240 +2
=========================================
Files 1156 1156
Lines 176924 176934 +10
Branches 3391 3392 +1
=========================================
+ Hits 104543 104555 +12
+ Misses 69015 69014 -1
+ Partials 3366 3365 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
R: @Abacn |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
shall we instead make a lock? Initialize a ServiceLoader everytime on method call sounds expensive |
Changed to lazy loading with |
| SCHEMA_INFORMATION_PROVIDERS = new AtomicReference<>(); | ||
|
|
||
| private static List<SchemaInformationProvider> getSchemaInformationProviders() { | ||
| return SCHEMA_INFORMATION_PROVIDERS.updateAndGet( |
There was a problem hiding this comment.
this makes List only update once. I'm not sure if it is possible if iterating over "SchemaInformationProviders.INSTANCE" would return different values overtime currently
forwarding to experts who I remember reviewed / fixed concurrency issues R: @scwhittle @kennknowles thanks!
There was a problem hiding this comment.
I'm also not sure when providers will be registered. From ServiceLoader docs it caches internally but looks for new things to load each time.
It seems like a smaller change to semantics to synchronize access to the loader.
Can you add a static Object to use as a lock, annotate INSTANCE as GuardedBy it and synchronize iteration?
| } | ||
| } catch (Exception e) { | ||
| LOG.debug("No Schema information found for type {}", outputType, e); |
There was a problem hiding this comment.
This message doesn't seem fully accurate, since we are not yet done checking for schemas, right?
|
Run Java_IOs_Direct PreCommit |
|
Run Java PreCommit |
| SCHEMA_INFORMATION_PROVIDERS = new AtomicReference<>(); | ||
|
|
||
| private static List<SchemaInformationProvider> getSchemaInformationProviders() { | ||
| return SCHEMA_INFORMATION_PROVIDERS.updateAndGet( |
There was a problem hiding this comment.
I'm also not sure when providers will be registered. From ServiceLoader docs it caches internally but looks for new things to load each time.
It seems like a smaller change to semantics to synchronize access to the loader.
Can you add a static Object to use as a lock, annotate INSTANCE as GuardedBy it and synchronize iteration?
| @@ -85,57 +100,58 @@ public ConvertedSchemaInformation( | |||
| public static <T> ConvertedSchemaInformation<T> getConvertedSchemaInformation( | |||
| Schema inputSchema, TypeDescriptor<T> outputType, SchemaRegistry schemaRegistry) { | |||
|
|
|||
| ConvertedSchemaInformation<T> schemaInformation = null; | |||
| ConvertedSchemaInformation<T> schemaInformation; | |||
There was a problem hiding this comment.
nit: move this definition to where assigned
There was a problem hiding this comment.
In practice, new classes would have to be loaded / added to classpath. I think the way we use service loaders is fixed when things are launched, unless someone is getting quite fancy.
Fixes #30606
Successful run: https://github.com/Amar3tto/beam/actions/runs/13314417715
Reason:
Instances of
ServiceLoadclass are not safe for use by multiple concurrent threads. And we had singletonprivate static final ServiceLoader<SchemaInformationProvider> INSTANCE.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.