Skip to content

HDDS-4680. Change default OM Node ID from UUID to a constant#1793

Merged
hanishakoneru merged 3 commits intoapache:masterfrom
hanishakoneru:HDDS-4680
Jan 15, 2021
Merged

HDDS-4680. Change default OM Node ID from UUID to a constant#1793
hanishakoneru merged 3 commits intoapache:masterfrom
hanishakoneru:HDDS-4680

Conversation

@hanishakoneru
Copy link
Contributor

What changes were proposed in this pull request?

If a nodeID is not set explicitly (for a single node OM cluster), then it defaults to the OM storage ID which is an UUID string. Proposing to change this default to a constant (such as "om1") instead.
This would help when a cluster is upgraded to HA for example. It is not straightforward to change the nodeID after Ratis server has been instantiated (as the nodeID is used to generate the RaftPeerID). Hence, it would be good to have a more readable nodeID by default.

Note that existing single node Ratis enabled OM clusters will have to set the nodeId explicitly if upgraded.

What is the link to the Apache JIRA

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

How was this patch tested?

Manually tested.

@hanishakoneru hanishakoneru requested a review from arp7 January 13, 2021 23:08
public static final String OM_DEFAULT_NODE_ID = "om1";

// Dummy OMNodeID for OM Clients to use for a non-HA OM setup
public static final String OM_NODE_ID_DUMMY = "omNodeIdDummy";
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this omNodeIdDummy will only be use if someone disables Ratis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this would be used for single node Ratis also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't understand then - how do we use both OM_DEFAULT_NODE_ID and OM_NODE_ID_DUMMY with single node Ratis?

Copy link
Contributor Author

@hanishakoneru hanishakoneru Jan 14, 2021

Choose a reason for hiding this comment

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

OM_NODE_ID_DUMMY was being used just as a dummy nodeId in OMFailoverProxyProvider for non-HA cluster.
But now that we have a default, we should remove the dummy. It would not affect the client in anyway as this is only for non-HA.
Thanks for pointing this out Arpit. I will update the PR.

Copy link
Contributor

@arp7 arp7 left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@arp7 arp7 left a comment

Choose a reason for hiding this comment

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

+1

@hanishakoneru hanishakoneru merged commit 6fe3e8a into apache:master Jan 15, 2021
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Mar 11, 2021
…1793)

(cherry picked from commit 6fe3e8a)
Change-Id: Iabdc34cd4e6f3a7515ae63f4b0e7ce0953a497e3
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.

2 participants

Comments