[CALCITE-6048] ServerTest#testTruncateTable fails intermittently due …#3567
[CALCITE-6048] ServerTest#testTruncateTable fails intermittently due …#3567libenchao wants to merge 2 commits intoapache:mainfrom
Conversation
…to method not found exception
|
Kudos, SonarCloud Quality Gate passed! |
| if (method == null) { | ||
| if (map.containsKey(key)) { | ||
| // We already looked for the method and found nothing. | ||
| // We should get again because it may be putted an object by another thread. |
There was a problem hiding this comment.
Does this solve the race or just reduce the window of vulnerability?
The right way to do this is with some locking mechanism.
There was a problem hiding this comment.
You are right, adding synchronizing was in my first version. However, after looking at all the failure history, I only see the wrong result, not see the concurrent accessing exceptions, so I just proposed to add a minimal improvement without adding too much synchronizing burden to it.
I don't have a strong opinion on it, if you think adding locking mechanism is a more thorough solution which we should do, I can change back to it.
There was a problem hiding this comment.
I don't really understand where the threads come from, so I have no idea how to do this.
If all it takes is to make this method synchronized (or some other one), it looks more principled.
If you think this works always, I say it's fine.
There was a problem hiding this comment.
The multi-threading comes from tests concurrent running, and they share a singleton ServerDdlExecutor, which will result concurrent accessing here.
I agree that adding synchronized is a better choice, since we already have some similar usages such as in JaninoRelMetadataProvider.
|
It seems that the CI running time has not changed. LGTM. |
3 participants
…to method not found exception