Skip to content

Conversation

@nandakumar131
Copy link
Contributor

@nandakumar131 nandakumar131 commented Mar 29, 2019

PR #620 brings in new ReplicationManager, this change is to refactor ContainerReportProcessing logic in SCM so that it complements ReplicationManager and plugin the new ReplicationManager code.

@nandakumar131 nandakumar131 requested a review from arp7 March 29, 2019 11:19
@hadoop-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 32 Docker mode activated.
_ Prechecks _
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 7 new or modified test files.
_ trunk Compile Tests _
0 mvndep 26 Maven dependency ordering for branch
+1 mvninstall 1088 trunk passed
+1 compile 81 trunk passed
+1 checkstyle 26 trunk passed
+1 mvnsite 74 trunk passed
+1 shadedclient 727 branch has no errors when building and testing our client artifacts.
+1 findbugs 114 trunk passed
+1 javadoc 59 trunk passed
_ Patch Compile Tests _
0 mvndep 10 Maven dependency ordering for patch
+1 mvninstall 74 the patch passed
+1 compile 76 the patch passed
+1 javac 76 the patch passed
+1 checkstyle 24 the patch passed
+1 mvnsite 59 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 721 patch has no errors when building and testing our client artifacts.
+1 findbugs 120 the patch passed
+1 javadoc 53 the patch passed
_ Other Tests _
+1 unit 66 common in the patch passed.
+1 unit 97 server-scm in the patch passed.
+1 asflicense 25 The patch does not generate ASF License warnings.
3603
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-662/1/artifact/out/Dockerfile
GITHUB PR #662
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 05c20b2ef81f 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / f41f938
maven version: Apache Maven 3.3.9
Default Java 1.8.0_191
findbugs v3.1.0-RC1
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-662/1/testReport/
Max. process+thread count 416 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-hdds/server-scm U: hadoop-hdds
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-662/1/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

LOG.info("Starting Replication Monitor Thread.");
running = true;
replicationMonitor.start();
CompletableFuture.runAsync(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use the forkJoin commonPool. It has very few threads and can be easily exhausted. We saw this to be a frequent issue in unit tests.

Instead use the overload that accepts an Executor.

" in {} milliseconds.", delay);
Thread.sleep(delay);
} catch (InterruptedException ignored) {
// InterruptedException is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set interrupted flag here.

}
}
final Set<ContainerID> missingReplicas = new HashSet<>(containersInSCM);
missingReplicas.removeAll(containersInDn);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the last few lines are doing? Didn't quite get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we have to identify the replicas which are missing from the reported datanode.

SCM (NodeManager) has the list of container replicas that are expected in a given datanode, we identify the missing replicas by finding the delta between the list maintained in NodeManager and the list of replicas reported by the datanode.

Once we identify the missing replicas, we go and update the replica map in ContainerManager.

* @param replicas List of ContainerReplicaProto
* @param publisher EventPublisher reference
*/
private void updateDeleteTransaction(final DatanodeDetails datanodeDetails,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few questions on this function. I am not familiar with this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for updating the block deletion transaction Id reported by the container replica. If the container replica is lagging behind in block deletion, we trigger PENDING_DELETE_STATUS event so that SCMBlockDeletingService can resend block deletion commands to the datanode.

@hadoop-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 25 Docker mode activated.
_ Prechecks _
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 7 new or modified test files.
_ trunk Compile Tests _
0 mvndep 30 Maven dependency ordering for branch
+1 mvninstall 1050 trunk passed
+1 compile 74 trunk passed
+1 checkstyle 27 trunk passed
+1 mvnsite 79 trunk passed
+1 shadedclient 769 branch has no errors when building and testing our client artifacts.
+1 findbugs 110 trunk passed
+1 javadoc 55 trunk passed
_ Patch Compile Tests _
0 mvndep 10 Maven dependency ordering for patch
+1 mvninstall 68 the patch passed
+1 compile 66 the patch passed
+1 javac 66 the patch passed
+1 checkstyle 22 the patch passed
+1 mvnsite 59 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 720 patch has no errors when building and testing our client artifacts.
+1 findbugs 124 the patch passed
+1 javadoc 58 the patch passed
_ Other Tests _
+1 unit 67 common in the patch passed.
+1 unit 100 server-scm in the patch passed.
+1 asflicense 28 The patch does not generate ASF License warnings.
3592
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-662/2/artifact/out/Dockerfile
GITHUB PR #662
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 26351f9c94b1 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 8b6deeb
maven version: Apache Maven 3.3.9
Default Java 1.8.0_191
findbugs v3.1.0-RC1
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-662/2/testReport/
Max. process+thread count 434 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-hdds/server-scm U: hadoop-hdds
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-662/2/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

Copy link
Contributor

@arp7 arp7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 lgtm!

@nandakumar131 nandakumar131 merged commit 48a58bc into apache:trunk Apr 4, 2019
@nandakumar131 nandakumar131 deleted the HDDS-1207 branch April 4, 2019 11:03
shanthoosh pushed a commit to shanthoosh/hadoop that referenced this pull request Oct 15, 2019
This Request is a copy of apache#647(got garbled). This PR  already addresses all the comments brought up in the other request.

Author: Boris S <[email protected]>
Author: Boris S <[email protected]>
Author: Boris Shkolnik <[email protected]>

Reviewers: Shanthoosh Venkatraman <[email protected]>, Prateek Maheshwari <[email protected]>

Closes apache#662 from sborya/NewConsumerAdmin2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants