Skip to content

Conversation

@YuanbenWang
Copy link
Contributor

What changes were proposed in this pull request?

When decommissioning or recommissioning a large number of DNs, current command will be so long because a long list of hosts must be included.
So we should support to get hosts from file when decommissioning or recommissioning DNs.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tests.

@sodonnel
Copy link
Contributor

If we add this for decommission and recommission, we should also add for the MaintenanceSubCommand too.

I also wonder if passing a filename is the best approach, or would it be better to read from stdin? Eg, in InfoSubCommand I recently changed it in #5659 to take a list of containers either on the command line or read from stdin if "-" was passed. It would be good if we had consistency across the commands.

@errose28
Copy link
Contributor

+1 for stdin instead of a file as a more flexible approach

@YuanbenWang
Copy link
Contributor Author

If we add this for decommission and recommission, we should also add for the MaintenanceSubCommand too.

I also wonder if passing a filename is the best approach, or would it be better to read from stdin? Eg, in InfoSubCommand I recently changed it in #5659 to take a list of containers either on the command line or read from stdin if "-" was passed. It would be good if we had consistency across the commands.

@sodonnel Thank you so much for your constructive feedback on my PR! I will definitely take the suggestions into consideration and use stdin instead of file reading.

@YuanbenWang YuanbenWang changed the title HDDS-10139. Support to get hosts from file when decommissioning or recommissioning DNs. HDDS-10139. Support to get hosts from stdin when DN is decommissioning, recommissioning or entering maintenance. Jan 18, 2024
@YuanbenWang
Copy link
Contributor Author

@sodonnel I have updated the code according to your suggestions. Could you please help review it? Thanks.

Copy link
Contributor

@sodonnel sodonnel left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for implementing the suggestion!

@sodonnel sodonnel merged commit 5da02ba into apache:master Jan 19, 2024
@YuanbenWang YuanbenWang deleted the HDDS-10139 branch January 19, 2024 11:01
@YuanbenWang
Copy link
Contributor Author

@sodonnel @errose28 Thanks for the code review.

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants