HDDS-2671 Have NodeManager.getNodeStatus throw NodeNotFoundException

[sodonnel]

Requires HDDS-2593 to be committed before this one.

What changes were proposed in this pull request?

Currently, the SCM node manager method getNodeStatus catches any NodeNotFoundException and returns null.

  /**
   * Returns the node status of a specific node.
   *
   * @param datanodeDetails Datanode Details
   * @return NodeStatus for the node
   */
  @Override
  public NodeStatus getNodeStatus(DatanodeDetails datanodeDetails) {
    try {
      return nodeStateManager.getNodeStatus(datanodeDetails);
    } catch (NodeNotFoundException e) {
      // TODO: should we throw NodeNotFoundException?
      return null;
    }
  }

This should throw the exception to ensure downstream code does not need to perform a null check each time it is called.

What is the link to the Apache JIRA

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

How was this patch tested?

Tested by existing unit tests which make calls to this API.

[adoroszlai]

LGTM, with minor suggestion for improvement inline.

[adoroszlai]

I think this should be more specific, eg. IllegalStateException seems to be a good candidate. If you agree, please remember to update the corresponding catch, too.

[sodonnel]

Yea that makes sense. This means that is a "real" runtime exception occurs, the catch will not stop it propagating up the stack and instead we only catch this more specific exception. I have made this change.

[adoroszlai]

Shouldn't this method really throw NodeNotFoundException if dni is not found, instead of returning a healthy status, to more closely reflect actual NodeManager behavior?

[sodonnel]

Yea it would make more sense for this method do thow NodeNotFoundException, however, I cheated somewhat when using this class in an earlier patch within TestReplicationManager. Replication manager only calls the "getNodeStatus()" method of the NodeManager, and to avoid having to change too many existing tests I defaulted this to return "Healthy + Inservice" for any node not registered so the existing tests passed without modification.

I have an existing Jira HDDS-2673 to refactor and merge MockNodeManager and SimpleMockNodeManager into one, so perhaps we could improve this issue as part of that?

[adoroszlai]

perhaps we could improve this issue as part of that?

Sure, that's fine, too.

[sodonnel]

I added a comment to HDDS-2673 to remind about this when I get to working on it.

[adoroszlai]

Thanks @sodonnel for updating the patch.