[feat][ci] Enable assertions when running tests in CI#19653
Closed
michaeljmarshall wants to merge 1 commit intoapache:masterfrom
Closed
[feat][ci] Enable assertions when running tests in CI#19653michaeljmarshall wants to merge 1 commit intoapache:masterfrom
michaeljmarshall wants to merge 1 commit intoapache:masterfrom
Conversation
michaeljmarshall
added
area/test
doc-not-needed
michaeljmarshall
requested review from
codelipenghui,
eolivelli,
lhotari and
nicoloboschi
lhotari
requested changes
Feb 28, 2023
Member
lhotari
left a comment
lhotari
left a comment
There was a problem hiding this comment.
MAVEN_OPTS isn't used for forked test processes. The maven-surefire-plugin will run tests in a separate process.
There's no need to separately enable assertions since maven-surefire-plugin enables all assertions by default (doc).
We can close this PR since it's not needed.
Member
Author
|
@lhotari - great, thanks for sharing those docs. |
1 task
michaeljmarshall
added a commit
that referenced
this pull request
Mar 20, 2023
…9670) ### Motivation The `DirectProxyHandler` and the `ProxyConnection` are run in the same event loop to prevent context switching. As such, we do not need to schedule an event onto an event loop that is in fact the same event loop. Further, scheduling on that event loop could have resulted in uncaught failures because the method was run without any error handling. Additionally, we can use `assert` to verify that we are in the event loop. Netty makes extensive use of this paradigm, as described in this PR #19653. The primary benefit here is that we skip some unnecessary volatile reads when running the code in production. ### Modifications * Replace `checkState` with `assert` in `ProxyConnection` class * Remove unnecessary event execution in callback when starting up the `DirectProxyHandler`. ### Verifying this change This change is covered by the assertions that are added. ### Does this pull request potentially affect one of the following parts: This is a minor improvement that should not break anything. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: michaeljmarshall#33
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.
Motivation
I noticed recently that Netty uses asserts to ensure that certain methods are run on the correct thread. This seems like a reasonable usage to me given that this kind of thing is hard to verify with a test and easy to verify at runtime. Here are some Netty usage references:
https://github.com/netty/netty/blob/d34212439068091bcec29a8fad4df82f0a82c638/transport/src/main/java/io/netty/channel/PendingWriteQueue.java#L68
https://github.com/netty/netty/blob/c18fc2bc68c08805b2553336a3bf02f0c8c50972/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java#L210
https://github.com/netty/netty/blob/00905d64c817b87aa37dee1c2bc3292b874b47a6/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2LocalFlowController.java#L179
And many more that might change over time.
As such, I recently committed some assertions to the Pulsar code base:
pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
Lines 838 to 839 in de43ad0
If we want to use these assertions, I think we must enable them in our CI pipeline to ensure the assertions are actually verified.
Modifications
MAVEN_OPTSto include the flag that will enable assertions for allorg.apache.pulsarsub packages.Verifying this change
I am not exactly sure how to verify this with a test, so I'll need to figure that out.
Documentation
doc-not-neededI don't think we need docs yet, but if we want to make an official stance on when to use
assert, maybe we should document it.Matching PR in forked repository
This should not break tests.