Fix releasePartialSequences value propagation for SequenceSizeReleaseStrategy #10004
Conversation
There was a problem hiding this comment.
Play attention how title of this PR is broken.
Our preferences is to have a first line of the first commit message not longer that 50 symbols. That one becomes a PR title, plus it is easy to read commit history for GIt.
There is also an article by our former colleague explaining an importance of good commit message: https://cbea.ms/git-commit/
| @@ -173,7 +175,7 @@ public void testAggPerfDefaultPartial() throws InterruptedException, ExecutionEx | |||
|
|
|||
| Collection<?> result = resultFuture.get(10, TimeUnit.SECONDS); | |||
| assertThat(result).isNotNull(); | |||
| assertThat(result.size()).isEqualTo(120000); | |||
| assertThat(result.size()).isEqualTo(1); | |||
There was a problem hiding this comment.
Why do you do a modification into a test if you don't enable it back?
There was a problem hiding this comment.
if I enable it, the value is equal to 1, so I change to 1 but keep it disabled. :)
I think we can also remove handler.setReleasePartialSequences(true); so it will be 12000.
There was a problem hiding this comment.
Well, I think the may reason behind disabling this test is its timing.
If it is takes like 5 seconds, then it is not OK.
We may indeed remodel it for a smaller number of items to aggregate though.
There was a problem hiding this comment.
Just nit-pick for this line:
assertThat(result).hasSize(1);
However, I worry about about the reasoning behind this test.
Looks like its idea is to demonstrate a performance improvement.
Therefore talking about partial sequence is some kind of copy/paste artifact.
And if we end up just expecting a one item in the end, then it is not about performance for the whole sequence.
So, my conclusion is just to remove that handler.setReleasePartialSequences(true); from this test and enable it back.
Looks like on my laptop this tests takes just only 2,5 seconds.
There was a problem hiding this comment.
yes, I run this method more than 10 times this afternoon, only 1 time very slow, morn than 1min.
cam I still mark it as Disabled, and remove handler.setReleasePartialSequences(true);
There was a problem hiding this comment.
Sure!
Let's keep it disabled, but remove the config which is not related to the logic if the test!
There was a problem hiding this comment.
Please, consider to follow DCO: https://spring.io/blog/2025/01/06/hello-dco-goodbye-cla-simplifying-contributions-to-spring.
Otherwise, we even cannot review your PR, not talking about accepting.
Fixes: spring-projects#10003 Issue Link: spring-projects#10003 Signed-off-by: Jiandong Ma <[email protected]>
There was a problem hiding this comment.
Looks like DCO complains that suggestion sign-off by is not what your Git user provides:
Author: Jiandong, Committer: Jiandong; Expected "Jiandong [[email protected]](mailto:[email protected])", but got "Jiandong Ma [[email protected]](mailto:[email protected])".
true, my IDEA commit panel and terminal use different email account ... |
| @@ -173,7 +175,7 @@ public void testAggPerfDefaultPartial() throws InterruptedException, ExecutionEx | |||
|
|
|||
| Collection<?> result = resultFuture.get(10, TimeUnit.SECONDS); | |||
| assertThat(result).isNotNull(); | |||
| assertThat(result.size()).isEqualTo(120000); | |||
| assertThat(result.size()).isEqualTo(1); | |||
There was a problem hiding this comment.
Just nit-pick for this line:
assertThat(result).hasSize(1);
However, I worry about about the reasoning behind this test.
Looks like its idea is to demonstrate a performance improvement.
Therefore talking about partial sequence is some kind of copy/paste artifact.
And if we end up just expecting a one item in the end, then it is not about performance for the whole sequence.
So, my conclusion is just to remove that handler.setReleasePartialSequences(true); from this test and enable it back.
Looks like on my laptop this tests takes just only 2,5 seconds.
Signed-off-by: Jiandong Ma <[email protected]>
Signed-off-by: Jiandong Ma <[email protected]>
Fixes: #10003
Issue Link: #10003