Skip to content

use random:uniform instead of os:pid when constructing node name in nodetool#868

Merged
tsloughter merged 3 commits into
erlware:masterfrom
hmmr:patch-1
Aug 25, 2021
Merged

use random:uniform instead of os:pid when constructing node name in nodetool#868
tsloughter merged 3 commits into
erlware:masterfrom
hmmr:patch-1

Conversation

@hmmr

@hmmr hmmr commented May 11, 2021

Copy link
Copy Markdown
Contributor

Borrowing from https://github.com/basho/node_package/blob/4.0/priv/base/nodetool#L195, this is to help reduce the risk of hitting the atom table limit, as was reported by one of our customers who was calling riak-admin continuously and frequently enough to trigger the atom table overflow.

…in append_node_suffix

Borrowing from https://github.com/basho/node_package/blob/4.0/priv/base/nodetool#L195, this will help reduce the risk of hitting the atom table limit, as was reported by one of our customers who was calling riak-admin continuously and frequently enough to trigger the atom table overflow.
@tsloughter

Copy link
Copy Markdown
Member

This looks good, thanks. I'll probably merge soon.

But I wanted to note that in OTP 23+ nodetool is no longer used and this issue does not exist. Obviously still worth it to be fixed for those using pre-23, just wanted to mention it :)

@hmmr

hmmr commented May 11, 2021

Copy link
Copy Markdown
Contributor Author

@tsloughter Indeed, I read that note and slightly pondered if it's worth while bothering. But, on reflection, it seems it still does :)

@tsloughter

Copy link
Copy Markdown
Member

Hm, the shelltestrunner tests and the tests on windows fail.

@Bob-The-Marauder Bob-The-Marauder left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should random be replaced by rand as random is deprecated?

@ferd

ferd commented May 12, 2021

Copy link
Copy Markdown
Collaborator

Yes, definitely should replace with the newer stuff where available.

hmmr added a commit to hmmr/relx that referenced this pull request May 24, 2021
Instances of nodetool generate random node name suffxes to facilitate running multiple simultaneous calls in parallel.  However, each time nodetool connects to the target node, a new atom is created on the latter.  If this happens frequently and/or long enough, it will eventually crash the node as it hits the atom table limit.  As a workaround, if the caller can guarantee calls are serialized and isolated in time, defining an env variable $NODETOOL_NODE_PREFIX will create identical atoms for node name prefix, thus avoiding generation of new atoms.

The proposed change is complimentary to erlware#868, aiming to address the issue, reported by one of our customers, in which a riak node hit the atom table limit (yes, all of 1M+ entries) and crashed. A postmortem showed the table filled with `[email protected]`, accumulated over a period of time resulting from calls to `riak admin status` every 5 min.

Note that I did not attempt to do any changes that may need to be done, to the same effect, in extended_bin_windows, as it's not straightforward for me which they would be (my knowledge of scripting in Windows is some 30 year old).
@hmmr

hmmr commented Jul 15, 2021

Copy link
Copy Markdown
Contributor Author

@tsloughter After the approval, what is the current state of this PR? Is there anything I can do to help?

@tsloughter

Copy link
Copy Markdown
Member

Hey, sorry about that. I don't know what the hell is going on with CI... there is at least 1 other PR that should be passing CI but isn't that I also want to merge and cut a release with.

@tsloughter

Copy link
Copy Markdown
Member

Could you repush so it kicks of CI again? There isn't even a "rerun" option anywhere like there usually is...

@hmmr

hmmr commented Jul 16, 2021

Copy link
Copy Markdown
Contributor Author

Once it's in, there's a more substantial #871.

@tsloughter
tsloughter merged commit e6c3ae4 into erlware:master Aug 25, 2021
@hmmr
hmmr deleted the patch-1 branch August 27, 2021 13:14
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.

4 participants