Skip to content
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.

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

DiscoveryJoiner was using WAIT_SECONDS_BEFORE_JOIN where MAX_WAIT_SECONDS_BEFORE_JOIN was meant #23083

Closed
wants to merge 1 commit into from

Conversation

jglick
Copy link

@jglick jglick commented Dec 5, 2022

For example

maxWaitMillisBeforeJoin = node.getProperties().getMillis(ClusterProperty.MAX_WAIT_SECONDS_BEFORE_JOIN);

looks right but here the variable was not named with max so I suspect this was simply a typo at some point.
Observed while experimenting with a custom DiscoveryStrategy whose start method was getting called but discoverNodes was not. In a debugger I tracked this down to DiscoveryJoiner.getPossibleAddressesForInitialJoin skipping the call to getPossibleAddresses. The problem disappeared when I removed

config.setProperty(ClusterProperty.WAIT_SECONDS_BEFORE_JOIN.getName(), "0");

No idea how this sort of thing would be tested, sorry.

@hz-devops-test hz-devops-test added the Source: Community PR or issue was opened by a community user label Dec 5, 2022
@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

11 similar comments
@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

devOpsHazelcast commented Dec 5, 2022

CLA assistant check
All committers have signed the CLA.

@jglick
Copy link
Author

jglick commented Oct 10, 2023

I believe this remains valid. However

Please reopen with comments

is not possible since I lack permission to reopen. Help @frant-hartm as last committer?

@@ -49,7 +49,7 @@ public class DiscoveryJoiner

public DiscoveryJoiner(Node node, DiscoveryService discoveryService, boolean usePublicAddress) {
super(node);
this.maximumWaitingTimeBeforeJoinSeconds = node.getProperties().getInteger(WAIT_SECONDS_BEFORE_JOIN);
this.maximumWaitingTimeBeforeJoinSeconds = node.getProperties().getInteger(MAX_WAIT_SECONDS_BEFORE_JOIN);
Copy link
Contributor

Choose a reason for hiding this comment

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

hello @jglick 👋

My memory is hazy about this particular change, but I reckon that if you change this field to use MAX_WAIT_SECONDS_BEFORE_JOIN then it will increase the initial startup time from +-5s to +-20s for any discovery strategy except UDP and static TCPs. That's probably not what you want. I agree the field name is misleading, it should have been called just "waitingTimeBeforeJoinSeconds` or so.

Copy link
Author

Choose a reason for hiding this comment

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

Čau! #23083 (comment) since that was previously hidden. If the code here is intentional, I suppose at least a comment is in order? (Understood that this was years ago.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree that the field has been mis-named here and would keep the WAIT_SECONDS_BEFORE_JOIN property. @hasancelik wdyt?

@jglick

This comment was marked as outdated.

@vbekiaris vbekiaris reopened this Oct 11, 2023
@vbekiaris
Copy link
Contributor

@jglick I reopened the PR and restored the original description (apologies on behalf of our bot , it should not misbehave like that in the future).

@vbekiaris
Copy link
Contributor

run-lab-run

@devOpsHazelcast
Copy link
Collaborator

PR closed by Hazelcast automation as no activity (>6 months). Please reopen with comments, if necessary. Thank you for using Hazelcast and your valuable contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automation: PR auto closed Source: Community PR or issue was opened by a community user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants