-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29828 Upgrade TestIPC related tests to junit5 #7626
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
Conversation
|
While rewriting and debugging, I found a possible problem is that, for testTimeout and testAsyncTimeout, when the test method finishes, the rpc call is still executing(sleeping), so when it finishes, it will record a tracing span in the global opentelemetry instance. In JUnit4, since we always reset the global opentelemetry instance, so this may cause exception describe there. And in JUnit5, we do not reset the global instance but the timeout call may add new spans after we clear all the spans for the previous test and cause the next test to fail because of unexpected spans. So the main fix is that, I added a check before shutting down the rpc server in testTimeout and testAsyncTimeout, to wait until all the pending calls are finished. In this way I think the test will be more stable. @ndimiduk FYI. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
left a comment
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.
This pattern from OTEL is annoying.
Just for a counter-idea, what if we add a notification mechanism to TestProtobufRpcServiceImpl#pause() , the test method registers a Future that is completed on the way out of the method.
| } | ||
| } finally { | ||
| // wait until all active calls quit, otherwise it may mess up the tracing spans | ||
| await().atMost(Duration.ofSeconds(2)) |
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.
I hate to bring it up, but would 5 sec be more reliable in our CI?
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.
The timeout value is 1 secs, so I think 2 secs is enough?
Our rpc implementation does not support this... |
|
I was thinking just for the purpose of keeping the test reliable. It's all within the same process, so the main test thread would wait until notified by the end of the pause. Complete the future within a finally block and it should be enough. But maybe my understanding here is superficial. Thanks for digging here Duo. |
Rpc calls run in a thread pool at server side, and we will use the same process to run all the test methods, so there is no simple way for us to wait in main thread. We can think the current fix in this PR is to make the main thread join on the thread pool for running rpc calls at server side... |
Signed-off-by: Nick Dimiduk <[email protected]> (cherry picked from commit 2c8d1c0)
Signed-off-by: Nick Dimiduk <[email protected]> (cherry picked from commit 2c8d1c0)
Signed-off-by: Nick Dimiduk <[email protected]> (cherry picked from commit 2c8d1c0)
Signed-off-by: Nick Dimiduk <[email protected]> (cherry picked from commit 2c8d1c0)
No description provided.