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/10/20 15:10:28 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
sodonnel opened a new pull request, #3867:
URL: https://github.com/apache/ozone/pull/3867
## What changes were proposed in this pull request?
In [HDDS-6567](https://issues.apache.org/jira/browse/HDDS-6567), we stored the datanode queued command counts into DatanodeInfo on each heartbeat. However, we did not consider the commands that were queued to goto the Datanode on that heartbeat. The real count, is the count queued on the DNs, plus the commands queued in SCM we are about to send to the DN over the heartbeat being processed.
This Jira adds the "commands ready to send" count to the stored counts to give a better reflection on the current queued count on the DN.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7370
## How was this patch tested?
Adapted existing tests
--
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] aswinshakil commented on a diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1006135381
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -305,8 +314,19 @@ public void setCommandCounts(CommandQueueReportProto cmds) {
"Setting it to zero", cmdCount, this);
cmdCount = 0;
}
+ cmdCount += mutableCmds.getOrDefault(command, 0);
+ // Each CommandType will be in the report once only. So we remove any
+ // we have seen, so we can add anything the DN has not reported but
+ // there is a command queued for. The DNs should return a count for all
+ // command types even if they have a zero count, so this is really to
+ // handle something being wrong on the DN where it sends a spare report.
+ // It really should never happe.
Review Comment:
```suggestion
// It really should never happen.
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -286,11 +286,20 @@ public void setNodeStatus(NodeStatus newNodeStatus) {
* Set the current command counts for this datanode, as reported in the last
* heartbeat.
* @param cmds Proto message containing a list of command count pairs.
+ * @param commandsToBeSent Summary of commands which will be sent to the DN
+ * as the heartbeat is processed and should be added
+ * to the command count.
*/
- public void setCommandCounts(CommandQueueReportProto cmds) {
+ public void setCommandCounts(CommandQueueReportProto cmds,
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent) {
try {
int count = cmds.getCommandCount();
+ Map<SCMCommandProto.Type, Integer> mutableCmds
+ = new HashMap<>(commandsToBeSent);
lock.writeLock().lock();
+ // Purge the existing counts, as each report should completely replace
+ // the existing counts.
+ commandCounts.clear();
Review Comment:
Just a quick question, why was this not done before and done now? I understand we would like to keep the current count and replace the stale counters. I thought it was already done by `commandCounts.put(command, cmdCount)` and also now with `commandCounts.putAll(mutableCmds)`.
--
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 diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1006189303
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -286,11 +286,20 @@ public void setNodeStatus(NodeStatus newNodeStatus) {
* Set the current command counts for this datanode, as reported in the last
* heartbeat.
* @param cmds Proto message containing a list of command count pairs.
+ * @param commandsToBeSent Summary of commands which will be sent to the DN
+ * as the heartbeat is processed and should be added
+ * to the command count.
*/
- public void setCommandCounts(CommandQueueReportProto cmds) {
+ public void setCommandCounts(CommandQueueReportProto cmds,
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent) {
try {
int count = cmds.getCommandCount();
+ Map<SCMCommandProto.Type, Integer> mutableCmds
+ = new HashMap<>(commandsToBeSent);
lock.writeLock().lock();
+ // Purge the existing counts, as each report should completely replace
+ // the existing counts.
+ commandCounts.clear();
Review Comment:
Yea not sure why I didn't have that before. I guess we could omit it, but I should not do any harm either.
--
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] aswinshakil commented on a diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1006184832
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -286,11 +286,20 @@ public void setNodeStatus(NodeStatus newNodeStatus) {
* Set the current command counts for this datanode, as reported in the last
* heartbeat.
* @param cmds Proto message containing a list of command count pairs.
+ * @param commandsToBeSent Summary of commands which will be sent to the DN
+ * as the heartbeat is processed and should be added
+ * to the command count.
*/
- public void setCommandCounts(CommandQueueReportProto cmds) {
+ public void setCommandCounts(CommandQueueReportProto cmds,
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent) {
try {
int count = cmds.getCommandCount();
+ Map<SCMCommandProto.Type, Integer> mutableCmds
+ = new HashMap<>(commandsToBeSent);
lock.writeLock().lock();
+ // Purge the existing counts, as each report should completely replace
+ // the existing counts.
+ commandCounts.clear();
Review Comment:
Thanks for the detailed explanation. My initial doubt was `commandCounts.clear();` was added now in this patch, I was just wondering why wasn't it added before. I guess the initial thought was that the command not reported by DN should be 0, it would be cleared anyway (with 0) by the `commandCounts.put(command, cmdCount)`.
The changes look good to me.
--
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] DaveTeng0 commented on a diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
DaveTeng0 commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1001271796
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestCommandQueueReportHandler.java:
##########
@@ -77,35 +79,51 @@ public void resetEventCollector() throws IOException {
public void testQueueReportProcessed() throws NodeNotFoundException {
DatanodeDetails dn = MockDatanodeDetails.randomDatanodeDetails();
nodeManager.register(dn, null, null);
+
+ // Add some queued commands to be sent to the DN on this heartbeat. This
+ // means the real queued count will be the value reported plus the commands
+ // sent to the DN.
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent = new HashMap<>();
+ commandsToBeSent.put(SCMCommandProto.Type.valueOf(1), 2);
+ commandsToBeSent.put(SCMCommandProto.Type.valueOf(2), 1);
+
SCMDatanodeHeartbeatDispatcher.CommandQueueReportFromDatanode
- commandReport = getQueueReport(dn);
+ commandReport = getQueueReport(dn, commandsToBeSent);
commandQueueReportHandler.onMessage(commandReport, this);
int commandCount = commandReport.getReport().getCommandCount();
for (int i = 0; i < commandCount; i++) {
int storedCount = nodeManager.getNodeQueuedCommandCount(dn,
commandReport.getReport().getCommand(i));
- Assertions.assertEquals(commandReport.getReport().getCount(i),
- storedCount);
+ int expectedCount = commandReport.getReport().getCount(i);
+ // For the first two commands, we added extra commands
Review Comment:
Hello Stephen! Just for my self-learning, why the first two commands we add extra commands~? Thanks~ :pray:
--
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 #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
sodonnel merged PR #3867:
URL: https://github.com/apache/ozone/pull/3867
--
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 diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1006168779
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -286,11 +286,20 @@ public void setNodeStatus(NodeStatus newNodeStatus) {
* Set the current command counts for this datanode, as reported in the last
* heartbeat.
* @param cmds Proto message containing a list of command count pairs.
+ * @param commandsToBeSent Summary of commands which will be sent to the DN
+ * as the heartbeat is processed and should be added
+ * to the command count.
*/
- public void setCommandCounts(CommandQueueReportProto cmds) {
+ public void setCommandCounts(CommandQueueReportProto cmds,
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent) {
try {
int count = cmds.getCommandCount();
+ Map<SCMCommandProto.Type, Integer> mutableCmds
+ = new HashMap<>(commandsToBeSent);
lock.writeLock().lock();
+ // Purge the existing counts, as each report should completely replace
+ // the existing counts.
+ commandCounts.clear();
Review Comment:
I'm not sure I follow your question.
There are two counts. The ones reported from the DN on its heartbeart - these are queued on the DN.
Then there are commands queued in SCM ready to be sent to the DN as part of processing the heartbeat.
After the heartbeat, the real queue on the DN is the sum of the two (queued on DN + queued on SCM).
Before this change, we only stored the "queued on the DN count". This count is not yet used anywhere, but I missed adding the commands the heartbeat will add to the queue, so the old count is not correct.
The line `commandCounts.put(command, cmdCount)` should do all the updates as per the comment in the code. The line `commandCounts.putAll(mutableCmds)` is a "catch all" incase the DN is not reporting counts for a command that is queued on SCM - it shouldn't happen in practice. I have also removed the entry from mutableCommands after adding 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] sodonnel commented on a diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1001492492
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestCommandQueueReportHandler.java:
##########
@@ -77,35 +79,51 @@ public void resetEventCollector() throws IOException {
public void testQueueReportProcessed() throws NodeNotFoundException {
DatanodeDetails dn = MockDatanodeDetails.randomDatanodeDetails();
nodeManager.register(dn, null, null);
+
+ // Add some queued commands to be sent to the DN on this heartbeat. This
+ // means the real queued count will be the value reported plus the commands
+ // sent to the DN.
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent = new HashMap<>();
+ commandsToBeSent.put(SCMCommandProto.Type.valueOf(1), 2);
+ commandsToBeSent.put(SCMCommandProto.Type.valueOf(2), 1);
+
SCMDatanodeHeartbeatDispatcher.CommandQueueReportFromDatanode
- commandReport = getQueueReport(dn);
+ commandReport = getQueueReport(dn, commandsToBeSent);
commandQueueReportHandler.onMessage(commandReport, this);
int commandCount = commandReport.getReport().getCommandCount();
for (int i = 0; i < commandCount; i++) {
int storedCount = nodeManager.getNodeQueuedCommandCount(dn,
commandReport.getReport().getCommand(i));
- Assertions.assertEquals(commandReport.getReport().getCount(i),
- storedCount);
+ int expectedCount = commandReport.getReport().getCount(i);
+ // For the first two commands, we added extra commands
Review Comment:
This is just to test the new part of the code - that the queued commands in the list are added to the totals in the report count. I did not add extra commands to all of them, so I could see the others work ok when there isn't extra commands.
--
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] DaveTeng0 commented on a diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
DaveTeng0 commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1002270632
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestCommandQueueReportHandler.java:
##########
@@ -77,35 +79,51 @@ public void resetEventCollector() throws IOException {
public void testQueueReportProcessed() throws NodeNotFoundException {
DatanodeDetails dn = MockDatanodeDetails.randomDatanodeDetails();
nodeManager.register(dn, null, null);
+
+ // Add some queued commands to be sent to the DN on this heartbeat. This
+ // means the real queued count will be the value reported plus the commands
+ // sent to the DN.
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent = new HashMap<>();
+ commandsToBeSent.put(SCMCommandProto.Type.valueOf(1), 2);
+ commandsToBeSent.put(SCMCommandProto.Type.valueOf(2), 1);
+
SCMDatanodeHeartbeatDispatcher.CommandQueueReportFromDatanode
- commandReport = getQueueReport(dn);
+ commandReport = getQueueReport(dn, commandsToBeSent);
commandQueueReportHandler.onMessage(commandReport, this);
int commandCount = commandReport.getReport().getCommandCount();
for (int i = 0; i < commandCount; i++) {
int storedCount = nodeManager.getNodeQueuedCommandCount(dn,
commandReport.getReport().getCommand(i));
- Assertions.assertEquals(commandReport.getReport().getCount(i),
- storedCount);
+ int expectedCount = commandReport.getReport().getCount(i);
+ // For the first two commands, we added extra commands
Review Comment:
got it! thanks Stephen!
--
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 diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1006168779
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -286,11 +286,20 @@ public void setNodeStatus(NodeStatus newNodeStatus) {
* Set the current command counts for this datanode, as reported in the last
* heartbeat.
* @param cmds Proto message containing a list of command count pairs.
+ * @param commandsToBeSent Summary of commands which will be sent to the DN
+ * as the heartbeat is processed and should be added
+ * to the command count.
*/
- public void setCommandCounts(CommandQueueReportProto cmds) {
+ public void setCommandCounts(CommandQueueReportProto cmds,
+ Map<SCMCommandProto.Type, Integer> commandsToBeSent) {
try {
int count = cmds.getCommandCount();
+ Map<SCMCommandProto.Type, Integer> mutableCmds
+ = new HashMap<>(commandsToBeSent);
lock.writeLock().lock();
+ // Purge the existing counts, as each report should completely replace
+ // the existing counts.
+ commandCounts.clear();
Review Comment:
I'm not sure I follow your question.
There are two counts. The ones reported from the DN on its heartbeart - these are queued on the DN.
Then there are commands queued in SCM ready to be sent to the DN as part of processing the heartbeat.
After the heartbeat, the real queue on the DN is the sum of the two (queued on DN + queued on SCM).
Before this change, we only stored the "queued on the DN count". This count is not yet used anywhere, but I missed adding the commands the heartbeat will add to the queue, so the old count is not correct.
The line `commandCounts.put(command, cmdCount)` should do all the updates as per the comment in the code. The line `commandCounts.putAll(mutableCmds)` is a "catch all" incase the DN is not reporting counts for a command that is queued on SCM - it shouldn't happen in practice. I have also removed the entry from mutableCommands after adding it to the cmdCount, so in practice mutableCommands should be empty by the time it gets to the putAll() call.
--
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