Optimize CachingLocalSegmentAllocator#getSequenceName#8909
Optimize CachingLocalSegmentAllocator#getSequenceName#8909jon-wei merged 8 commits intoapache:masterfrom
Conversation
|
I didn't find any tests for this class. Since the logic is very trivial I decided to skip adding a unit test for this. cc @jihoonson |
ccaominh
left a comment
ccaominh
left a comment
There was a problem hiding this comment.
Interval.toString() generates an ISO string, so I don't think it's affected by the format locale.
LGTM 👍
|
Would you fix the style issue? Also, would you mind adding a comment why it's not using |
Will do! Chi's comment about toString() generating the correct format had me a little worried so I'm adding a simple unit test. Fighting with the styling guidelines right now. Should have another patch out within a day |
There was a problem hiding this comment.
@ccaominh I copied most these helper initialization functions from your DefaultCaching...AllocatorTest in the refactoring PR
If you want I can consolidate them after your change is merged
There was a problem hiding this comment.
Yeah, probably good to consolidate.
|
It appears that the EmitterTest is stuck |
|
I believe |
Replace StringUtils#format with string addition to generate the sequence name for an interval and partition. This is faster because format uses a Matcher under the covers to replace the string format with the variables.
suneet-s
force-pushed
the
sequence-name-opt
branch
from
52df535 to
9d29652
Compare
|
It doesn't look like the failures are related to this change 🤔 @ccaominh @jihoonson could you take a look? |
|
The "license check" job needs to be retriggered (there was a failure when generating the license report). I'm not sure why the "security vulnerabilities" job failed. It's flagging CVE-2018-10237, which has a CVSS of 5.9, but the job is configured to only fail on CVSS 7 or higher. The job may need to be retriggered as well. |
| } | ||
|
|
||
| @Test | ||
| public void test_getSequenceName_forIntervalAndRow_shouldUseISOFormatAndPartitionNumForRow() |
There was a problem hiding this comment.
Thanks for adding tests. I think we prefer to use a short but intuitive name for tests and add comments if necessary.
There was a problem hiding this comment.
can you recommend a name? I like this format because it's explicit about what the test is doing
test_<functionUnderTest>_<condition>_<expectedResult>
There was a problem hiding this comment.
Hmm, I would just use getSequenceName() for these tests with a comment for the expected value because they look simple enough to understand.
can you recommend a name? I like this format because it's explicit about what the test is doing
test_<functionUnderTest>_<condition>_<expectedResult>
I agree on that we could have a better format or a better practice for test names (we've been talking about a better practice for testing here). Unfortunately, we don't have such a format or practice and I think it could be better to make them consistent for now because we don't know what kind of policy we would adopt yet. I think it would be worth to start a discussion on it.
There was a problem hiding this comment.
Ok I've renamed the tests to the less explicit format and left a comment for how I'd rename them in the future. When the tests pass, can you merge?
How do I start this discussion? The linked discussion is about a lot of different testing practices and I think driving consensus on such a large topic will take a long time. How do I start a discussion thread about naming practices. Who needs to approve the change to the naming format? And who has veto rights?
There was a problem hiding this comment.
Sorry for late reply. You can start a discussion by starting a new thread on dev which is you already did (thanks!). There's no approval or veto in this kind of discussion. Anyone can participate in the discussion and express their opinion. The discussion could be concluded when the community accepts it.
|
+1 for driving to a conclusion about test name format. FWIW, I like
Suneet's explicit format.
…On Tue, Dec 17, 2019, 5:17 PM Jihoon Son ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
indexing-service/src/test/java/org/apache/druid/indexing/common/task/RangePartitionCachingLocalSegmentAllocatorTest.java
<#8909 (comment)>
:
> @@ -141,6 +142,16 @@ public void allocatesCorrectShardSpecsForLastPartition()
testAllocate(row, interval, partitionNum, null);
}
+ @test
+ public void test_getSequenceName_forIntervalAndRow_shouldUseISOFormatAndPartitionNumForRow()
Hmm, I would just use getSequenceName() for these tests with a comment
for the expected value because they look simple enough to understand.
can you recommend a name? I like this format because it's explicit about
what the test is doing
test_<functionUnderTest>_<condition>_<expectedResult>
I agree on that we could have a better format or a better practice for
test names (we've been talking about a better practice for testing here
***@***.***/msg02426.html>).
Unfortunately, we don't have such a format or practice and I think it could
be better to make them consistent for now because we don't know what kind
of policy we would adopt yet. I think it would be worth to start a
discussion on it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8909>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPSYCVA3LVQOFZ7KI4HAPLQZF22TANCNFSM4JPINE2Q>
.
|
5 participants
Fixes #8904
Description
Replace StringUtils#format with string addition to generate the sequence
name for an interval and partition. This is faster because format uses a
Matcher under the covers to replace the string format with the variables.
There is likely a performance gain here because this function is called for
every row in InputSourceProcessor#process
This PR has: