[HBASE-22606] : BucketCache additional tests #324
Conversation
|
🎊 +1 overall
This message was automatically generated. |
|
|
||
| validateGetPartitionSize(cache, 0.1f, 0.5f); | ||
| validateGetPartitionSize(cache, 0.7f, 0.5f); | ||
| validateGetPartitionSize(cache, 0.2f, 0.5f); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Should be @test (expected = IllegalArgumentException.class)
| conf.setFloat(BucketCache.MEMORY_FACTOR_CONFIG_NAME, 0.2f); | ||
| boolean isTestCompleted = false; | ||
| try { | ||
| new BucketCache(ioEngineName, Long.MAX_VALUE, 1, constructedBlockSizes, writeThreads, |
There was a problem hiding this comment.
Instead of surrounding it by a try/catch, and adding extra boolean variable, you can just add _ (expected = IllegalArgumentException.class)_ to @test annotation (see my comment above)
There was a problem hiding this comment.
Sure @wchevreuil Although I wanted to ensure the message of IllegalArgumentException is validated. You think it's fine to do?
There was a problem hiding this comment.
Even though we need to update upper limit of cache capacity, it would be better to update the message as well. Do you agree with this Exception message check @wchevreuil ?
There was a problem hiding this comment.
Right, I guess it's worth to validate the message indeed. In this case, though, could you use assertEquals in the catch, failing the test right after BucketCache constructor is called (after all, we do want an IAE to be thrown)?
There was a problem hiding this comment.
@wchevreuil Thanks a lot for your suggestion. Updated the PR with assertEquals for message. However, kept the boolean variable since it can help us ensure that IAE was definitely thrown. Without checking boolean value, we can't know whether IAE was thrown.
| // reconfig buckets sizes, the biggest bucket is small than constructedBlockSize (8k or 16k) | ||
| // so it can't restore cache from file | ||
| int[] smallBucketSizes = new int[] { 2 * 1024 + 1024, 4 * 1024 + 1024 }; | ||
| int[] smallBucketSizes = new int[]{2 * 1024 + 1024, 4 * 1024 + 1024}; |
There was a problem hiding this comment.
For testing BucketCache with no persistence, it does not seem we need this extra check anyways, as newly created BCs with no persistence will always be empty (regardless of the passed bucket sizes).
There was a problem hiding this comment.
Makes sence. Let me pass empty array then
| bucketCache.shutdown(); | ||
| assertTrue(new File(persistencePath).exists()); | ||
| int[] smallBucketSizes = new int[]{2 * 1024 + 1024, 4 * 1024 + 1024}; |
There was a problem hiding this comment.
Why not "new int[]{3 * 1024, 5 * 1024 };", instead? Would be easier to read, IMO. Also, I would think this and next 4 lines would be worth place on its own test method, since it's validating extra scenario where when bucket cache is instantiated with smaller bucket sizes, it can't read previously saved buckets.
| @@ -114,8 +113,8 @@ | |||
| /** Priority buckets */ | |||
| @VisibleForTesting | |||
| static final float DEFAULT_SINGLE_FACTOR = 0.25f; | |||
| static final float DEFAULT_MULTI_FACTOR = 0.50f; | |||
| static final float DEFAULT_MEMORY_FACTOR = 0.25f; | |||
| private static final float DEFAULT_MULTI_FACTOR = 0.50f; | |||
There was a problem hiding this comment.
For purely cosmetic/checkstyle fixes, could we do it on a separate jira?
There was a problem hiding this comment.
@wchevreuil since the checkstyle fixes are very less, is it fine to send them with this PR? I would take care of this going forward
There was a problem hiding this comment.
Updated this PR with only unit tests. Will create separate JIRA for cosmetic/checkstyle fixes
There was a problem hiding this comment.
Yeah, having specific jiras for each fix is generally preferred.
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HBASE-22606-master
branch
from
4547eca to
cf76691
Compare
virajjasani
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| try { | ||
| new BucketCache(ioEngineName, Long.MAX_VALUE, 1, constructedBlockSizes, writeThreads, | ||
| writerQLen, null, 100, conf); | ||
| } catch (IllegalArgumentException e) { |
There was a problem hiding this comment.
You don't need the boolean if you just add a fail("should had thrown IAE!") call right here after the constructor, but before the catch block. If the constructor does not throw IAE, it will reach this line and fail the test. If any other runtime exception is thrown, it will give a test error anyways, since we are not defining it as expected in the @test annotation.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@wchevreuil the branch has all master commits, not sure why the build is giving -1 for patch. |
No description provided.