Skip to content

Conversation

@Tejaskriya
Copy link
Contributor

What changes were proposed in this pull request?

We currently have a log message in NetworkTopologyImpl set at the WARN level for when no datanodes are available. It’ll usually show up with a bunch of other related, repeating logs. It is better to set it to DEBUG level, and relying on the INFO message at the end as a summary of what happened.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9051

How was this patch tested?

Only log level change. No testing required.

Copy link
Contributor

@tanvipenumudy tanvipenumudy left a comment

Choose a reason for hiding this comment

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

Thank you @Tejaskriya for the patch.

It is better to set it to DEBUG level, and relying on the INFO message at the end as a summary of what happened

Can you please specify the INFO message which should suffice instead of the WARN messages?

@Tejaskriya
Copy link
Contributor Author

Can you please specify the INFO message which should suffice instead of the WARN messages?

The method chooseNodeInternal() in which this log is present is finally being called by SCMContainerPlacementRackAware#chooseNode() and SCMContainerPlacementRackScatter#chooseNode() . Both these methods have Log.info() messages to log "No satisfied datanode" being found after maxTries are hit. These INFO logs will serve the purpose of showing us the summary

Copy link
Contributor

@siddhantsangwan siddhantsangwan left a comment

Choose a reason for hiding this comment

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

@Tejaskriya The purpose here is to remove redundant information in the info logs. To do this, we need to ensure all classes using this method from NetworkTopology are providing a summary or similar information so that we can get rid of the repeated warnings from this class. SCMContainerPlacementRackScatter already does this. What about other classes?

@siddhantsangwan
Copy link
Contributor

Thinking about it some more, it's actually hard to guarantee what a future user of the class will do, and the implementation details of this class cannot really rely on that. I think we're better off just bringing down the log level from WARN to INFO.

@Tejaskriya
Copy link
Contributor Author

I think we're better off just bringing down the log level from WARN to INFO.

We want to bring it down from WARN to INFO and not DEBUG?

@siddhantsangwan
Copy link
Contributor

We want to bring it down from WARN to INFO and not DEBUG?

Yes

@Tejaskriya
Copy link
Contributor Author

@siddhantsangwan thank you for the review! I have changed the log level to INFO now. Could you please approve the workflows if the patch is good to go?

@siddhantsangwan
Copy link
Contributor

Thanks for the update, I've started CI.

@adoroszlai adoroszlai merged commit d2eb1ed into apache:master Jan 20, 2024
@adoroszlai
Copy link
Contributor

Thanks @Tejaskriya for the patch, @siddhantsangwan, @tanvipenumudy for the review.

Tejaskriya added a commit to Tejaskriya/ozone that referenced this pull request Jan 24, 2024
@Tejaskriya Tejaskriya deleted the HDDS-9051 branch February 5, 2024 05:02
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.

4 participants