You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/02/14 13:18:45 UTC
[GitHub] [ozone] sodonnel opened a new pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
sodonnel opened a new pull request #3085:
URL: https://github.com/apache/ozone/pull/3085
## What changes were proposed in this pull request?
The container report handing code has some issues which this Jira intends to address.
When handling full container reports, we make several copies of large sets to identify containers in SCM, but not reported by the DN (they have somehow got lost on the DNs).
Looking at the current code:
```
synchronized (datanodeDetails) {
final List<ContainerReplicaProto> replicas =
containerReport.getReportsList();
final Set<ContainerID> containersInSCM =
nodeManager.getContainers(datanodeDetails);
final Set<ContainerID> containersInDn = replicas.parallelStream()
.map(ContainerReplicaProto::getContainerID)
.map(ContainerID::valueOf).collect(Collectors.toSet());
final Set<ContainerID> missingReplicas = new HashSet<>(containersInSCM);
missingReplicas.removeAll(containersInDn);
processContainerReplicas(datanodeDetails, replicas, publisher);
processMissingReplicas(datanodeDetails, missingReplicas);
/*
* Update the latest set of containers for this datanode in
* NodeManager
*/
nodeManager.setContainers(datanodeDetails, containersInDn);
containerManager.notifyContainerReportProcessing(true, true);
}
```
The Set "containersInSCM" comes from NodeStateMap:
```
public Set<ContainerID> getContainers(UUID uuid)
throws NodeNotFoundException {
lock.readLock().lock();
try {
checkIfNodeExist(uuid);
return Collections
.unmodifiableSet(new HashSet<>(nodeToContainer.get(uuid)));
} finally {
lock.readLock().unlock();
}
}
```
This returns a new copy of the set, so there is no need to wrap it UnModifiable. This means we can avoid copying it again in the report Handler. The current code ends up with 3 copies of this potentially large set.
Next we take the FCR and stream it into a set of ContainerID. This is used for two purposes - 1, to subtract from "containersInSCM" to yield the missing containers. 2, to replace the list already in nodeManager.
We can avoid this second large set, by simply adding each ContainerID to nodeManager if it is not already there and then remove any "missing" from nodeManager at the end. This also avoids replacing the entire set of ContainersIDs and all the tenured objects associated with it with new effectively identical object instances.
Finally we can process the replicas at the same time, and re-use the ContainerID object, which we currently form 2 times. We store one copy in the nodeManager, and then another distinct copy in each ContainerReplica.
I checked how many ContainerID objects are present in SCM, by loading 10 closed containers with 3 replicas, and capturing a heap histogram with jmap:
```
bash-4.2$ jmap -histo:live 8 | grep ContainerID
262: 81 1944 org.apache.hadoop.hdds.scm.container.ContainerID
301: 30 1440 org.apache.hadoop.hdds.scm.container.ContainerReplica
419: 10 800 org.apache.hadoop.hdds.scm.container.ContainerInfo
```
There are 10 Containers as expected, 30 replicas, but 81 ContainerID objects. Ideally there should be 10, to match the number of Containers, but some of these may be created from other code paths, such as pipeline creation.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6307
## How was this patch tested?
Existing tests cover the functionality.
For performance testing, I used the existing Freon SCMThroughputBenchmark class. With it, I simulated FCR handing for a single DN, reporting 10k, 20k and 30k containers. The original code performed approximately:
```
10k: 750 - 800ms per report.
20k: 1550 - 1650ms per report.
30k: 2300 - 2500ms per report.
```
The code in this PR:
```
10k: 530 - 570ms per report.
20k: 1100- 1200ms per report.
30k: 1550 - 1650ms per report.
```
Both scale proportionally with the number of containers, but the revised code is about 30% faster and has a reduced memory overhead.
After loading 30k containers with 1 replica each I checked the ContainerID object count using `jmap -histo:live`:
```
Original:
13: 120000 2880000 org.apache.hadoop.hdds.scm.container.ContainerID
Revised:
37: 30000 720000 org.apache.hadoop.hdds.scm.container.ContainerID
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808554961
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final Set<ContainerID> containersInSCM =
nodeManager.getContainers(datanodeDetails);
- final Set<ContainerID> containersInDn = replicas.parallelStream()
- .map(ContainerReplicaProto::getContainerID)
- .map(ContainerID::valueOf).collect(Collectors.toSet());
-
- final Set<ContainerID> missingReplicas = new HashSet<>(containersInSCM);
- missingReplicas.removeAll(containersInDn);
-
- processContainerReplicas(datanodeDetails, replicas, publisher);
- processMissingReplicas(datanodeDetails, missingReplicas);
-
- /*
- * Update the latest set of containers for this datanode in
- * NodeManager
- */
- nodeManager.setContainers(datanodeDetails, containersInDn);
-
+ for (ContainerReplicaProto replica : replicas) {
+ ContainerID cid = ContainerID.valueOf(replica.getContainerID());
+ ContainerInfo container = null;
+ try {
+ // We get the container using the ContainerID object we obtained
+ // from protobuf. However we don't want to store that object if
+ // there is already an instance for the same ContainerID we can
+ // reuse.
+ container = containerManager.getContainer(cid);
+ cid = container.containerID();
+ } catch (ContainerNotFoundException e) {
+ // Ignore this for now. It will be handle later with a null check.
+ }
+
+ boolean wasInSCM = containersInSCM.remove(cid);
Review comment:
```suggestion
boolean matchedToExpected = containersInSCM.remove(cid);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r814577771
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -219,7 +226,6 @@ public void updateLastUsedTime() {
public HddsProtos.ContainerInfoProto getProtobuf() {
HddsProtos.ContainerInfoProto.Builder builder =
HddsProtos.ContainerInfoProto.newBuilder();
- Preconditions.checkState(containerID > 0);
Review comment:
>=0 ? or just > should be suffice? I think id starts from 1 right?
Other left out check has >= validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#issuecomment-1051250228
@umamaheswararao I pushed a commit to address the nits you pointed out. Please check and approve if you are happy. Then when the CI goes green I can commit it. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808508324
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -133,8 +139,9 @@ public static ContainerInfo fromProtobuf(HddsProtos.ContainerInfoProto info) {
* {@link ContainerID} object.
*/
@Deprecated
Review comment:
Maybe drop the deprecated flag? Looks like this API is here to stay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#issuecomment-1042436850
Overall the change looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808468091
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -219,7 +226,6 @@ public void updateLastUsedTime() {
public HddsProtos.ContainerInfoProto getProtobuf() {
HddsProtos.ContainerInfoProto.Builder builder =
HddsProtos.ContainerInfoProto.newBuilder();
- Preconditions.checkState(containerID > 0);
Review comment:
Why drop this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808559316
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final Set<ContainerID> containersInSCM =
nodeManager.getContainers(datanodeDetails);
- final Set<ContainerID> containersInDn = replicas.parallelStream()
- .map(ContainerReplicaProto::getContainerID)
- .map(ContainerID::valueOf).collect(Collectors.toSet());
-
- final Set<ContainerID> missingReplicas = new HashSet<>(containersInSCM);
- missingReplicas.removeAll(containersInDn);
-
- processContainerReplicas(datanodeDetails, replicas, publisher);
- processMissingReplicas(datanodeDetails, missingReplicas);
-
- /*
- * Update the latest set of containers for this datanode in
- * NodeManager
- */
- nodeManager.setContainers(datanodeDetails, containersInDn);
-
+ for (ContainerReplicaProto replica : replicas) {
+ ContainerID cid = ContainerID.valueOf(replica.getContainerID());
+ ContainerInfo container = null;
+ try {
+ // We get the container using the ContainerID object we obtained
+ // from protobuf. However we don't want to store that object if
+ // there is already an instance for the same ContainerID we can
+ // reuse.
+ container = containerManager.getContainer(cid);
+ cid = container.containerID();
+ } catch (ContainerNotFoundException e) {
+ // Ignore this for now. It will be handle later with a null check.
+ }
+
+ boolean wasInSCM = containersInSCM.remove(cid);
+ if (!wasInSCM) {
Review comment:
I need to understand this better.. what is expected to happen if the container is null but was not in SCM's cache for the DN?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#issuecomment-1051292000
Changing the ContainerID check in the existing code from `>= 0` to `> 0`, has caused a lot of test failures. This is existing code, so I have put it back to how it was.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808552677
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final Set<ContainerID> containersInSCM =
nodeManager.getContainers(datanodeDetails);
- final Set<ContainerID> containersInDn = replicas.parallelStream()
- .map(ContainerReplicaProto::getContainerID)
- .map(ContainerID::valueOf).collect(Collectors.toSet());
-
- final Set<ContainerID> missingReplicas = new HashSet<>(containersInSCM);
- missingReplicas.removeAll(containersInDn);
-
- processContainerReplicas(datanodeDetails, replicas, publisher);
- processMissingReplicas(datanodeDetails, missingReplicas);
-
- /*
- * Update the latest set of containers for this datanode in
- * NodeManager
- */
- nodeManager.setContainers(datanodeDetails, containersInDn);
-
+ for (ContainerReplicaProto replica : replicas) {
+ ContainerID cid = ContainerID.valueOf(replica.getContainerID());
+ ContainerInfo container = null;
+ try {
+ // We get the container using the ContainerID object we obtained
+ // from protobuf. However we don't want to store that object if
+ // there is already an instance for the same ContainerID we can
+ // reuse.
+ container = containerManager.getContainer(cid);
+ cid = container.containerID();
+ } catch (ContainerNotFoundException e) {
+ // Ignore this for now. It will be handle later with a null check.
Review comment:
```suggestion
// Ignore this for now. It will be handled later with a null check.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808552263
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final Set<ContainerID> containersInSCM =
Review comment:
```suggestion
final Set<ContainerID> expectedContainerInDatanode =
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808982225
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final Set<ContainerID> containersInSCM =
nodeManager.getContainers(datanodeDetails);
- final Set<ContainerID> containersInDn = replicas.parallelStream()
- .map(ContainerReplicaProto::getContainerID)
- .map(ContainerID::valueOf).collect(Collectors.toSet());
-
- final Set<ContainerID> missingReplicas = new HashSet<>(containersInSCM);
- missingReplicas.removeAll(containersInDn);
-
- processContainerReplicas(datanodeDetails, replicas, publisher);
- processMissingReplicas(datanodeDetails, missingReplicas);
-
- /*
- * Update the latest set of containers for this datanode in
- * NodeManager
- */
- nodeManager.setContainers(datanodeDetails, containersInDn);
-
+ for (ContainerReplicaProto replica : replicas) {
+ ContainerID cid = ContainerID.valueOf(replica.getContainerID());
+ ContainerInfo container = null;
+ try {
+ // We get the container using the ContainerID object we obtained
+ // from protobuf. However we don't want to store that object if
+ // there is already an instance for the same ContainerID we can
+ // reuse.
+ container = containerManager.getContainer(cid);
+ cid = container.containerID();
+ } catch (ContainerNotFoundException e) {
+ // Ignore this for now. It will be handle later with a null check.
+ }
+
+ boolean wasInSCM = containersInSCM.remove(cid);
+ if (!wasInSCM) {
Review comment:
The logic before this change, always added the ContainerID to the nodemanager map, even if the container is not present in SCM. It is questionable on whether it should do that or not - it is hard to think of a reason why it is useful, but there might be one. As I am trying to improve the performance here, I would like to keep the logic the same so we don't introduce some unexpected side effect.
Looking at my new logic, we first set the ContainerID to the value obtained from the protobuf report. Then I update it to the value stored in the ContainerInfo if it exists, otherwise just use the one from proto. So this should work fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808550158
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
##########
@@ -70,30 +70,49 @@
this.logger = logger;
}
+ /**
+ *
+ * @param datanodeDetails DatanodeDetails for the DN
+ * @param replicaProto Protobuf representing the replicas
+ * @param publisher EventPublisher instance
+ * @throws IOException
+ * @throws InvalidStateTransitionException
+ */
+ protected void processContainerReplica(final DatanodeDetails datanodeDetails,
+ final ContainerReplicaProto replicaProto, final EventPublisher publisher)
+ throws IOException, InvalidStateTransitionException {
+ ContainerInfo container = getContainerManager().getContainer(
+ ContainerID.valueOf(replicaProto.getContainerID()));
+ processContainerReplica(
+ datanodeDetails, container, replicaProto, publisher);
+ }
+
/**
* Process the given ContainerReplica received from specified datanode.
*
* @param datanodeDetails DatanodeDetails of the node which reported
* this replica
+ * @param container ContainerInfo represending the container
* @param replicaProto ContainerReplica
+ * @param publisher EventPublisher instance
*
* @throws IOException In case of any Exception while processing the report
*/
protected void processContainerReplica(final DatanodeDetails datanodeDetails,
- final ContainerReplicaProto replicaProto, final EventPublisher publisher)
+ final ContainerInfo container, final ContainerReplicaProto replicaProto,
Review comment:
While you are at it. Nit: `
```suggestion
final ContainerInfo containerInfo, final ContainerReplicaProto replicaProto,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel merged pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel merged pull request #3085:
URL: https://github.com/apache/ozone/pull/3085
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808521994
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -219,7 +226,6 @@ public void updateLastUsedTime() {
public HddsProtos.ContainerInfoProto getProtobuf() {
HddsProtos.ContainerInfoProto.Builder builder =
HddsProtos.ContainerInfoProto.newBuilder();
- Preconditions.checkState(containerID > 0);
Review comment:
Inside the ContainerID constructor, the same pre-condition is there:
```
public ContainerID(long id) {
Preconditions.checkState(id >= 0,
"Container ID should be positive. %s.", id);
this.id = id;
}
```
So if we have a ContainerID instance inside this class, which we now must have rather than the long value, it must have a value greater than zero making this check redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r815128849
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -219,7 +226,6 @@ public void updateLastUsedTime() {
public HddsProtos.ContainerInfoProto getProtobuf() {
HddsProtos.ContainerInfoProto.Builder builder =
HddsProtos.ContainerInfoProto.newBuilder();
- Preconditions.checkState(containerID > 0);
Review comment:
This is existing code inside the ContainerID class - I have not changed it. The fact that things worked OK with > 0 in the report processing code implies container ID must start at 1. I can change this if you like.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r814969690
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
##########
@@ -70,30 +70,49 @@
this.logger = logger;
}
+ /**
+ *
Review comment:
You thought to add some description here and missed?
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -219,7 +226,6 @@ public void updateLastUsedTime() {
public HddsProtos.ContainerInfoProto getProtobuf() {
HddsProtos.ContainerInfoProto.Builder builder =
HddsProtos.ContainerInfoProto.newBuilder();
- Preconditions.checkState(containerID > 0);
Review comment:
>=0 ? or just > should be suffice? I think id starts from 1 right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808982861
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final Set<ContainerID> containersInSCM =
nodeManager.getContainers(datanodeDetails);
- final Set<ContainerID> containersInDn = replicas.parallelStream()
- .map(ContainerReplicaProto::getContainerID)
- .map(ContainerID::valueOf).collect(Collectors.toSet());
-
- final Set<ContainerID> missingReplicas = new HashSet<>(containersInSCM);
- missingReplicas.removeAll(containersInDn);
-
- processContainerReplicas(datanodeDetails, replicas, publisher);
- processMissingReplicas(datanodeDetails, missingReplicas);
-
- /*
- * Update the latest set of containers for this datanode in
- * NodeManager
- */
- nodeManager.setContainers(datanodeDetails, containersInDn);
-
+ for (ContainerReplicaProto replica : replicas) {
+ ContainerID cid = ContainerID.valueOf(replica.getContainerID());
+ ContainerInfo container = null;
+ try {
+ // We get the container using the ContainerID object we obtained
+ // from protobuf. However we don't want to store that object if
+ // there is already an instance for the same ContainerID we can
+ // reuse.
+ container = containerManager.getContainer(cid);
+ cid = container.containerID();
+ } catch (ContainerNotFoundException e) {
+ // Ignore this for now. It will be handle later with a null check.
+ }
+
+ boolean wasInSCM = containersInSCM.remove(cid);
Review comment:
I changed this, but I named the variable "alreadyInDn", which I think better describes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808552263
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode reportFromDatanode,
final Set<ContainerID> containersInSCM =
Review comment:
```suggestion
final Set<ContainerID> containersInDatanode =
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808520613
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -133,8 +139,9 @@ public static ContainerInfo fromProtobuf(HddsProtos.ContainerInfoProto info) {
* {@link ContainerID} object.
*/
@Deprecated
Review comment:
Yea, I think you could be correct. There is a need to get the numeric ID in a lot of places, so removing this is probably never going to happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on a change in pull request #3085: HDDS-6307. Improve processing and memory efficiency of full container reports
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r814577771
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
##########
@@ -219,7 +226,6 @@ public void updateLastUsedTime() {
public HddsProtos.ContainerInfoProto getProtobuf() {
HddsProtos.ContainerInfoProto.Builder builder =
HddsProtos.ContainerInfoProto.newBuilder();
- Preconditions.checkState(containerID > 0);
Review comment:
containerID >=0 ? or just > should be suffice? I think id starts from 1 right?
Other left out check has >= validation. Need to change to just > ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org