[ISSUE-257] RssMRUtils#getBlockId change the partitionId of int type to long #266
Conversation
|
Could you add some uts for this pr? |
|
Did this pr fix the issue #257? |
|
| @@ -312,7 +312,7 @@ ShuffleBlockInfo createShuffleBlock(SortWriteBuffer wb) { | |||
| final byte[] compressed = RssShuffleUtils.compressData(data); | |||
| 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? |
There was a problem hiding this comment.
Thanks @fpkgithub for digging into this issue. This change looks good.
Just some nitpicking here.
| @@ -74,6 +75,22 @@ public void blockConvertTest() { | |||
| 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.
|
@jerqi Please backport this fix into |
…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>
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