-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDDS-1146. Adding container related metrics in SCM. #1541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,7 @@ public void onMessage(final IncrementalContainerReportFromDatanode report, | |
LOG.debug("Processing incremental container report from data node {}", | ||
report.getDatanodeDetails().getUuid()); | ||
|
||
boolean success = true; | ||
for (ContainerReplicaProto replicaProto : | ||
report.getReport().getReportList()) { | ||
try { | ||
|
@@ -66,16 +67,25 @@ public void onMessage(final IncrementalContainerReportFromDatanode report, | |
nodeManager.addContainer(dd, id); | ||
processContainerReplica(dd, replicaProto); | ||
} catch (ContainerNotFoundException e) { | ||
success = false; | ||
LOG.warn("Container {} not found!", replicaProto.getContainerID()); | ||
} catch (NodeNotFoundException ex) { | ||
success = false; | ||
LOG.error("Received ICR from unknown datanode {} {}", | ||
report.getDatanodeDetails(), ex); | ||
} catch (IOException e) { | ||
success = false; | ||
LOG.error("Exception while processing ICR for container {}", | ||
replicaProto.getContainerID()); | ||
} | ||
} | ||
|
||
if (success) { | ||
getContainerManager().notifyContainerReportProcessing(false, true); | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason we are not doing getContainerManager().notifyContainerReportProcessing(false, success) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do that, just due to copy/paste from ICR code it is like this. (End result is same) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, we should change it to 1 line instead of if-else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed offline, will change this in later jira. For now we shall go with the current way. |
||
getContainerManager().notifyContainerReportProcessing(false, false); | ||
} | ||
|
||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, But I think we have a Enum that indicates if the report is full or partial. If there is already one, then we might want to use it. Otherwise, I am okay with the boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From proto, there is no enum. But i see we have TypedEvent
TypedEvent
INCREMENTAL_CONTAINER_REPORT = new TypedEvent<>(
IncrementalContainerReportFromDatanode.class,
"Incremental_Container_Report")
TypedEvent CONTAINER_REPORT =
new TypedEvent<>(ContainerReportFromDatanode.class, "Container_Report");
Do you mean we want to use them instead of boolean?