[ISSUE-257] RssMRUtils#getBlockId change the partitionId of int type to long#266
Conversation
|
Did this pr fix the issue #257? |
|
| final long crc32 = ChecksumUtils.getCrc32(compressed); | ||
| compressTime += System.currentTimeMillis() - start; | ||
| final long blockId = RssMRUtils.getBlockId(partitionId, taskAttemptId, getNextSeqNo(partitionId)); | ||
| final long blockId = RssMRUtils.getBlockId(Long.valueOf(partitionId), taskAttemptId, getNextSeqNo(partitionId)); |
There was a problem hiding this comment.
Long.valueof(partitionId) -> (long)partitionId
There was a problem hiding this comment.
ok , change to implicit type casting
There was a problem hiding this comment.
Is it necessary to change this line? Pass int value to long argument should be fine.
There was a problem hiding this comment.
Is it necessary to change this line? Pass int value to long argument should be fine.
Just tell the reader of code that we're sure that we need this type cast.
Codecov Report
@@ Coverage Diff @@
## master #266 +/- ##
=========================================
Coverage 59.70% 59.71%
Complexity 1377 1377
=========================================
Files 166 166
Lines 8916 8918 +2
Branches 853 853
=========================================
+ Hits 5323 5325 +2
Misses 3318 3318
Partials 275 275
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@fpkgithub Could you resolve the comment? |
roryqi
changed the title
kaijchen
left a comment
kaijchen
left a comment
There was a problem hiding this comment.
Thanks @fpkgithub for digging into this issue. This change looks good.
Just some nitpicking here.
| assertEquals(taskAttemptId, newTaskAttemptId); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Could use @ParameterizedTest here.
There was a problem hiding this comment.
For example (remove the outer for loop):
@ParameterizedTest
@ValueSource(ints = {0, 1, 233, 1023, 1024, 1234, (1 << Constants.PARTITION_ID_MAX_LENGTH) - 1})
public void partitionIdConvertBlockTest(int partitionId) {There was a problem hiding this comment.
I think it's ok now. Let's merge this pr first.
roryqi
left a comment
roryqi
left a comment
There was a problem hiding this comment.
@fpkgithub @kaijchen Thanks, LGTM
…to long (#266) ### What changes were proposed in this pull request? In the RssMRUtils#getBlockId method, change the partitionId of int type to long, make sure no overflow ### Why are the changes needed? 1: In the RssMRUtils#getBlockId method, change the partitionId of int type to long, make sure no overflow ### Does this PR introduce _any_ user-facing change? Fix #257 issue ,view this issue for details ### How was this patch tested? org.apache.hadoop.mapreduce.RssMRUtilsTest#partitionIdConvertBlockTest Co-authored-by: fengpeikun <kanas.feng@vipshop.com>
4 participants
What changes were proposed in this pull request?
Why are the changes needed?
1: In the RssMRUtils#getBlockId method, change the partitionId of int type to long, make sure no overflow
Does this PR introduce any user-facing change?
Fix #257 issue ,view this issue for details
How was this patch tested?
org.apache.hadoop.mapreduce.RssMRUtilsTest#partitionIdConvertBlockTest