Skip to content

HDDS-6290. operational-state and node-state options in datanode list CLI not working correctly #3105

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.

Sign up for GitHub

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

Merged
adoroszlai merged 1 commit into from
Feb 17, 2022
Merged

HDDS-6290. operational-state and node-state options in datanode list CLI not working correctly #3105

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 17 additions & 21 deletions ...-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/ListInfoSubcommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,28 +71,24 @@ public class ListInfoSubcommand extends ScmSubcommand {
@Override
public void execute(ScmClient scmClient) throws IOException {
pipelines = scmClient.listPipelines();
if (Strings.isNullOrEmpty(ipaddress) && Strings.isNullOrEmpty(uuid)) {
getAllNodes(scmClient).forEach(this::printDatanodeInfo);
} else {
Stream<DatanodeWithAttributes> allNodes = getAllNodes(scmClient).stream();
if (!Strings.isNullOrEmpty(ipaddress)) {
allNodes = allNodes.filter(p -> p.getDatanodeDetails().getIpAddress()
.compareToIgnoreCase(ipaddress) == 0);
}
if (!Strings.isNullOrEmpty(uuid)) {
allNodes = allNodes.filter(p ->
p.getDatanodeDetails().getUuidString().equals(uuid));
}
if (!Strings.isNullOrEmpty(nodeOperationalState)) {
allNodes = allNodes.filter(p -> p.getOpState().toString()
.compareToIgnoreCase(nodeOperationalState) == 0);
}
if (!Strings.isNullOrEmpty(nodeState)) {
allNodes = allNodes.filter(p -> p.getHealthState().toString()
.compareToIgnoreCase(nodeState) == 0);
}
allNodes.forEach(this::printDatanodeInfo);
Stream<DatanodeWithAttributes> allNodes = getAllNodes(scmClient).stream();
if (!Strings.isNullOrEmpty(ipaddress)) {
allNodes = allNodes.filter(p -> p.getDatanodeDetails().getIpAddress()
.compareToIgnoreCase(ipaddress) == 0);
Copy link
Contributor

@sodonnel sodonnel Feb 17, 2022

Choose a reason for hiding this comment

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

Do all these need a collect(...) at the end to form a new list and replace the old one, or is this logic filtering down the list correct as it stands?

Copy link
Contributor

@sodonnel sodonnel Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry - ignore my earlier comment. I missed that allNodes is a Stream and not a list at the top.

Do we need to have allNodes = allNodes.filter(...? Can we just drop the assignment so its like:

   ...
    if (!Strings.isNullOrEmpty(uuid)) {
      allNodes.filter(p ->
          p.getDatanodeDetails().getUuidString().equals(uuid));
    }
    if (!Strings.isNullOrEmpty(nodeOperationalState)) {
      allNodes.filter(p -> p.getOpState().toString()
          .compareToIgnoreCase(nodeOperationalState) == 0);
    }
   ...

Copy link
Contributor

@adoroszlai adoroszlai Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the assignment is needed. Stream.filter returns a new stream, and the original one can no longer be used.

jshell> var strings = Stream.of("a","aa","aaa","aaaa","aaaaa")
strings ==> java.util.stream.ReferencePipeline$Head@6ee52dcd

jshell> strings.filter(x -> x.length() > 2)
$22 ==> java.util.stream.ReferencePipeline$2@57e1b0c

jshell> strings.filter(x -> x.length() < 5)
|  Exception java.lang.IllegalStateException: stream has already been operated upon or closed
|        at AbstractPipeline.<init> (AbstractPipeline.java:203)

jshell> var strings = Stream.of("a","aa","aaa","aaaa","aaaaa")
strings ==> java.util.stream.ReferencePipeline$Head@480bdb19

jshell> strings = strings.filter(x -> x.length() < 5)
strings ==> java.util.stream.ReferencePipeline$2@2a742aa2

jshell> strings = strings.filter(x -> x.length() > 2)
strings ==> java.util.stream.ReferencePipeline$2@467aecef

jshell> strings.count()
$27 ==> 2

Copy link
Contributor

@sodonnel sodonnel Feb 17, 2022

Choose a reason for hiding this comment

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

Ah - thanks for clarifying. This is good to know!

}
if (!Strings.isNullOrEmpty(uuid)) {
allNodes = allNodes.filter(p ->
p.getDatanodeDetails().getUuidString().equals(uuid));
}
if (!Strings.isNullOrEmpty(nodeOperationalState)) {
allNodes = allNodes.filter(p -> p.getOpState().toString()
.compareToIgnoreCase(nodeOperationalState) == 0);
}
if (!Strings.isNullOrEmpty(nodeState)) {
allNodes = allNodes.filter(p -> p.getHealthState().toString()
.compareToIgnoreCase(nodeState) == 0);
}
allNodes.forEach(this::printDatanodeInfo);
}

private List<DatanodeWithAttributes> getAllNodes(ScmClient scmClient)
Expand Down