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

Set a default of 5m to recover_after_time when any of the expected*Nodes is set #6742

Closed
wants to merge 1 commit into from

Conversation

bleskes
Copy link
Contributor

@bleskes bleskes commented Jul 4, 2014

The recovery_after_time tells the gateway to wait before starting recovery from disk. The goal here is to allow for more nodes to join the cluster and thus not start potentially unneeded replications. The expectedNodes setting (and friends) tells the gateway when it can start recovering even if the recover_after_time has not yet elapsed. However, expectedNodes is useless if one doesn't set recovery_after_time. This commit changes that by setting a sensible default of 5m for recover_after_time if a expectedNodes setting is present.

assertThat(service.recoverAfterTime().millis(), Matchers.equalTo(timeValue.millis()));
}

private static class DummyClusterService implements ClusterService {
Copy link
Member

Choose a reason for hiding this comment

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

can we move this to our test infra? I think we do this in another place in our code (or one of our branches)

@kimchy
Copy link
Member

kimchy commented Jul 9, 2014

LGTM, except for a small comment on the test

@bleskes bleskes closed this in f480969 Jul 11, 2014
bleskes added a commit that referenced this pull request Jul 11, 2014
… `expected*Nodes` is set

The `recovery_after_time` tells the gateway to wait before starting recovery from disk. The goal here is to allow for more nodes to join the cluster and thus not start potentially unneeded replications. The `expectedNodes` setting (and friends) tells the gateway when it can start recovering even if the `recover_after_time` has not yet elapsed. However, `expectedNodes` is useless if one doesn't set `recovery_after_time`. This commit changes that by setting a sensible default of 5m for `recover_after_time` *if* a `expectedNodes` setting is present.

Closes #6742
@bleskes
Copy link
Contributor Author

bleskes commented Jul 11, 2014

I moved the cluster service noop class to our test infra package and pushed. Thx.

@bleskes bleskes deleted the recover_after_time_default branch July 11, 2014 09:30
@jpountz jpountz removed the review label Jul 16, 2014
@clintongormley clintongormley changed the title [Gateway] set a default of 5m to recover_after_time when any to the expected*Nodes is set Resiliency: Set a default of 5m to recover_after_time when any to the expected*Nodes is set Jul 16, 2014
@clintongormley clintongormley changed the title Resiliency: Set a default of 5m to recover_after_time when any to the expected*Nodes is set Resiliency: Set a default of 5m to recover_after_time when any of the expected*Nodes is set Jul 16, 2014
@clintongormley clintongormley added the :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. label Jun 7, 2015
@clintongormley clintongormley changed the title Resiliency: Set a default of 5m to recover_after_time when any of the expected*Nodes is set Set a default of 5m to recover_after_time when any of the expected*Nodes is set Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. >enhancement resiliency v1.3.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants