-
Notifications
You must be signed in to change notification settings - Fork 123
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
chore(spanner): unflake unit tests #3584
Conversation
7c8c91a
to
e552106
Compare
6986ab7
to
18a4e5b
Compare
27f72a3
to
62cdd43
Compare
7c869a5
to
46d6e7d
Compare
05b9d1b
to
22149da
Compare
b32f148
to
f54b562
Compare
f54b562
to
0541692
Compare
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.
Thanks, this looks promising! I have one question regarding whether this is actually a synchronization problem (so needing a lock), or just a caching problem (meaning; making it volatile should be enough).
// When start a new stream set the Span as current to make the gRPC Span a child of | ||
// this Span. | ||
stream = checkNotNull(startStream(resumeToken, streamMessageListener)); | ||
synchronized (monitor) { |
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.
Do we really need the lock in this method? As in: Is it really possible that two threads try to call this method simultaneously? Or is the problem just that there are two threads calling this method quickly after each other, and one of them does not (always) see the result of the previous call by another thread? And would in the latter case just making stream
volatile be enough?
The reason for asking is mainly because:
- If it is so that multiple threads can call this method simultaneously, from where does that happen? (If it is guaranteed that it does not happen in parallel, then the rest of this can be ignored)
- If that happens from both
computeNext()
andinitiateStreaming()
, then a potential follow-on problem could be that the call fromcomputeNext()
is executed first, and that will then not set a value forstreamMessageListener
. - And otherwise; If both parallel calls come through
initiateStreaming()
; Could we instead synchronize those calls? Or somehow prevent that from happening in parallel?
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.
volatile
didn't actually fix the issue. Let me try to figure out the issue at the root.
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.
Issue:
- We are trying to request for initial chunks in
startStream
which returns the stream. - We are trying to request chunks before stream object returned to
startGrpcStreaming
inResumableStreamIterator
- Because of this, chunk is received first and it created
ProduceRowsRunnable
thread and internally it calledGrpcResultSet#next()
and that internallyResumableStreamIterator#computeNext()
which creates the stream if it's null - Due to this reason, 2 begin transaction were started which is causing the the test failure.
@olavloite I have shared the events with you in chat which confirms this behaviour.
6e23a7d
to
e0cfeae
Compare
0b0c3a2
to
baf2b5a
Compare
6c54b63
to
aedcfd1
Compare
aedcfd1
to
273c45f
Compare
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.