HDFS-15940. Fixing and refactoring tests specific to Block recovery #2844
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
saintstack
reviewed
Mar 31, 2021
Thanks for the review @saintstack . The failures are flakies or some recent permanent ones. I have seen almost similar no of failures on many of recent PRs. Trying to spend some time when I can to fix them. |
|
There is a new build running. Lets see what set it produces. |
|
💔 -1 overall
This message was automatically generated. |
|
This is the usual no of failed tests that we have seen recently. Except for a couple of tests, majority of them seem to be failing quite often. I have seen them on other PRs too. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
changed the title
virajjasani
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
reviewed
Apr 3, 2021
There was a problem hiding this comment.
Thanx @virajjasani for volunteering the fix. Extremely sorry for coming up this late, occupied with some internal work.
In general everything looks good. Some minor nits. Please give a check, will conclude this post them. :-)
Comment on lines
125
to
111
| public class TestBlockRecovery { | ||
| public class TestBlockRecovery1 { |
There was a problem hiding this comment.
Lets not bother the name, Let it be as is.
There was a problem hiding this comment.
Sure, sounds good. Will update.
Comment on lines
339
to
328
| ReplicaRecoveryInfo replica1 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP-1, ReplicaState.FINALIZED); | ||
| ReplicaRecoveryInfo replica2 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP-2, ReplicaState.FINALIZED); | ||
| ReplicaRecoveryInfo replica1 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP - 1, ReplicaState.FINALIZED); | ||
| ReplicaRecoveryInfo replica2 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP - 2, ReplicaState.FINALIZED); |
There was a problem hiding this comment.
Can we chunk out the formatting changes in this file? We can restrict ourselves to only the intended changes, Usually we aren't mixing checkstyle fixes with the other code changes..
There was a problem hiding this comment.
Indeed I also did not want to mix checkstyle with actual test fix, however only after QA bot showed eol space and eol blank warnings, I updated this (in last 2 commits). Are you fine with it?
There was a problem hiding this comment.
May be because you changed the file name and it read it as a new file?
one or two fixes are ok, but in case the PR doesn't induce the checkstyle and there are plenty. we can let it stay as is, and let the one approving spend some time, checking that. :-)
Anyway lets ignore for now. I expect the build won't have anything which would require you any more commits
There was a problem hiding this comment.
Oh you are right, maybe QA bot had EOL issues which were not relevant to actual changes because I renamed the file earlier.
Anyway lets ignore for now. I expect the build won't have anything which would require you any more commits
Sure sounds good, I will take care of this going forward. It's the rename which might let QA bot scan entire file regardless of how minor the actual change is in that file.
...ect/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery2.java
Outdated
Show resolved
Hide resolved
virajjasani
commented
Apr 4, 2021
There was a problem hiding this comment.
Thanks for the review @ayushtkn , could you please take a look again?
Thanks
Comment on lines
125
to
111
| public class TestBlockRecovery { | ||
| public class TestBlockRecovery1 { |
There was a problem hiding this comment.
Sure, sounds good. Will update.
Comment on lines
339
to
328
| ReplicaRecoveryInfo replica1 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP-1, ReplicaState.FINALIZED); | ||
| ReplicaRecoveryInfo replica2 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP-2, ReplicaState.FINALIZED); | ||
| ReplicaRecoveryInfo replica1 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP - 1, ReplicaState.FINALIZED); | ||
| ReplicaRecoveryInfo replica2 = new ReplicaRecoveryInfo(BLOCK_ID, | ||
| REPLICA_LEN1, GEN_STAMP - 2, ReplicaState.FINALIZED); |
There was a problem hiding this comment.
Indeed I also did not want to mix checkstyle with actual test fix, however only after QA bot showed eol space and eol blank warnings, I updated this (in last 2 commits). Are you fine with it?
...ect/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery2.java
Outdated
Show resolved
Hide resolved
virajjasani
force-pushed
the
HDFS-15940-trunk
branch
from
259eccf to
db1524b
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
There are checkstyle warnings from the new file, |
…Contributed by Viraj Jasani
virajjasani
force-pushed
the
HDFS-15940-trunk
branch
from
0079954 to
71e9cf9
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
approved these changes
Apr 5, 2021
There was a problem hiding this comment.
Only a test change and the touched tests passes:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2844/13/testReport/org.apache.hadoop.hdfs.server.datanode/TestBlockRecovery2/
Will merge by EOD, if no objections
tasanuma
approved these changes
Apr 5, 2021
asfgit
pushed a commit
that referenced
this pull request
Apr 5, 2021
…2844). Contributed by Viraj Jasani Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
|
Thanx @virajjasani for the contribution and @tasanuma for the review!!! |
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
Apr 13, 2021
…pache#2844). Contributed by Viraj Jasani Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: Takanobu Asanuma <tasanuma@apache.org> (cherry picked from commit 2b5fd34) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
ferhui
pushed a commit
that referenced
this pull request
Apr 15, 2021
…2902) * HDFS-15940. Fixing and refactoring tests specific to Block recovery.(#2844). Contributed by Viraj Jasani (cherry picked from commit 2b5fd34) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java * HDFS-15940. Fix TestBlockRecovery2#testRaceBetweenReplicaRecoveryAndFinalizeBlock (ADDENDUM) (#2874) (cherry picked from commit 56bd968) (cherry picked from commit 4994b73) Co-authored-by: Viraj Jasani <vjasani@apache.org>
kiran-maturi
pushed a commit
to kiran-maturi/hadoop
that referenced
this pull request
Nov 24, 2021
…pache#2844). Contributed by Viraj Jasani Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.