Core: thread-safe initialization for GoogleAuthManager#16848
Open
vladislav-sidorovich wants to merge 1 commit into
Open
Core: thread-safe initialization for GoogleAuthManager#16848vladislav-sidorovich wants to merge 1 commit into
vladislav-sidorovich wants to merge 1 commit into
Conversation
vladislav-sidorovich
changed the title
anoopj
reviewed
Jun 17, 2026
| } | ||
|
|
||
| startLatch.countDown(); | ||
| finishLatch.await(10, TimeUnit.SECONDS); |
Member
There was a problem hiding this comment.
Probably should assert the return value of await() is true?
Contributor
Author
There was a problem hiding this comment.
I'm not 100% sure about it, let's discuss.
Java doc:
Returns:
true if the count reached zero and false if the waiting time elapsed before the count reached zero
So, in my opinion the test should fail on the verify(spyManager, times(1)) but not on the fact that the wait time > 10 seconds.
What do you think?
Note: actually in other tests in Iceberg it works without isTrue.
| } | ||
|
|
||
| startLatch.countDown(); | ||
| finishLatch.await(10, TimeUnit.SECONDS); |
Member
There was a problem hiding this comment.
Suggested change
| finishLatch.await(10, TimeUnit.SECONDS); | |
| assertThat(finishLatch.await(10, TimeUnit.SECONDS)).isTrue(); |
| GoogleAuthManager spyManager = spy(authManager); | ||
| doReturn(credentials) | ||
| .when(spyManager) | ||
| .loadCredentials(anyBoolean(), any(), anyBoolean(), any(), any()); |
Member
There was a problem hiding this comment.
The test probably won't catch the bug you fixed because it returns value quickly? In other words, does this test reliably fail without the fix?
Contributor
Author
There was a problem hiding this comment.
Yes, the test will fail. I commented the synchronyzed section and run the test. The test failed with the following error:
googleAuthManager.loadCredentials(
<any boolean>,
<any>,
<any boolean>,
<any>,
<any>
);
Wanted 1 time:
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.loadCredentials(GoogleAuthManager.java:128)
But was 10 times:
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
-> at org.apache.iceberg.gcp.auth.GoogleAuthManager.initialize(GoogleAuthManager.java:105)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make initialization for
GoogleAuthManagerthread-sage.