Skip to content

Conversation

@dombizita
Copy link
Contributor

What changes were proposed in this pull request?

In this change I fixed the handling of options when using ozone admin datanode list. Previously there was a condition check that only let using the --ip and --id options, in other cases it returned all the datanodes without filtering.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing tests (unit and acceptance) and tested in docker environment.

Stream<DatanodeWithAttributes> allNodes = getAllNodes(scmClient).stream();
if (!Strings.isNullOrEmpty(ipaddress)) {
allNodes = allNodes.filter(p -> p.getDatanodeDetails().getIpAddress()
.compareToIgnoreCase(ipaddress) == 0);
Copy link
Contributor

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

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

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

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!

@sodonnel
Copy link
Contributor

Thanks for spotting and fixing this issue. Could you extend the tests in TestListInfoSubCommand to reproduce this issue and then confirm the changes fix it please?

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @dombizita for the fix.

@adoroszlai adoroszlai merged commit 53fa44d into apache:master Feb 17, 2022
errose28 added a commit to errose28/ozone that referenced this pull request Feb 17, 2022
* master: (43 commits)
  HDDS-6212. SCM Container DB bootstrap on Recon startup for secure cluster (apache#3027)
  HDDS-6234. Repair containers affected by incorrect used bytes and block count. (apache#3042)
  HDDS-6262. ozone insight log stops working after OM DBUpdates message (apache#3044)
  HDDS-6290. operational-state and node-state options in datanode list CLI not working correctly (apache#3105)
  HDDS-6314. ConcurrentModificationException getting SCMContainerMetrics (apache#3101)
  HDDS-6284. Add BlockDeletingService worker size config (apache#3056)
  HDDS-6324. Do not trigger CI by reopening PR (apache#3092)
  HDDS-6283. Change ContainerStateMachine ContainerOpExecutor name (apache#3055)
  HDDS-6331. Remove toString in debug log parameters within SCMCommonPlacementPolicy (apache#3098)
  HDDS-6330. Remove unnecessary duplicate semicolons (apache#3097)
  HDDS-6305: Add metrics - number of FSO bucket creates (apache#3077)
  HDDS-6311. Fix number of keys displayed in Recon Overview. (apache#3081)
  HDDS-6325. Fix interface ClientProtocol methods typo setThreadLocalS3Auth and clearThreadLocalS3Auth (apache#3093)
  HDDS-6322. Fix Recon getting inccorrect sequenceNumber from OM (apache#3090)
  HDDS-5913. Avoid integer overflow when setting dfs.container.ratis.lo… (apache#2785)
  HDDS-6313. Remove replicas in ContainerStateMap when a container is deleted (apache#3086)
  HDDS-6186. Selective checks: skip integration check for unit test changes (apache#3061)
  HDDS-6310. Update json-smart to 2.4.7. (apache#3080)
  HDDS-6190. Cleanup unnecessary datanode id path checks. (apache#2993)
  HDDS-6304. Add enforcer to make sure ozone.version equals project.version (apache#3075)
  ...
@dombizita dombizita deleted the HDDS-6290 branch March 9, 2022 11:06
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.

3 participants