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

Expose WriteRequest.RefreshPolicy string representation #23106

Merged
merged 3 commits into from
Feb 13, 2017

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Feb 10, 2017

This commit changes the RefreshPolicy enum so that string representation are exposed. This will help the high level rest client to simply use refreshPolicy.getValue() to get the corresponding parameter value of a given refresh policy.

This commit changes the RefreshPolicy enum so that string representation are exposed. This will help the high level rest client to simply use  refreshPolicy.getValue() to get the corresponding parameter value of a given refresh policy.
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a couple of comments

@@ -65,45 +65,70 @@ default R setRefreshPolicy(String refreshPolicy) {
/**
* Don't refresh after this request. The default.
*/
NONE,
NONE((byte) 0, Boolean.FALSE.toString()),
Copy link
Member

Choose a reason for hiding this comment

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

nit: I am wondering if using "true" and "false" would be more readable, simply because I always forget if it's lowercase or uppercase.

WAIT_UNTIL;
WAIT_UNTIL((byte) 2, "wait_for");

private final byte id;
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 remove the id member and stick with the enum ordinal like we were doing before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we can :(

return policy;
}
}
throw new IllegalArgumentException("No cluster block level matching [" + id + "]");
Copy link
Member

Choose a reason for hiding this comment

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

is this a cluster block level?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a wonderful copy paste

@tlrx
Copy link
Member Author

tlrx commented Feb 10, 2017

Thanks @javanna, I updated the code.

}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeByte(getId());
}
out.writeByte((byte) ordinal()); }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: } is in a funny place.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM besides nik's comment

@tlrx tlrx merged commit de94c12 into elastic:master Feb 13, 2017
@tlrx tlrx deleted the update-refresh-policy branch February 13, 2017 09:49
@tlrx
Copy link
Member Author

tlrx commented Feb 13, 2017

Thanks @javanna @nik9000

@tlrx tlrx removed the review label Feb 13, 2017
tlrx added a commit that referenced this pull request Feb 13, 2017
This commit changes the RefreshPolicy enum so that string representation are exposed. This will help the high level rest client to simply use  refreshPolicy.getValue() to get the corresponding parameter value of a given refresh policy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants