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

Internal: Add support for sending cluster state using diffs #9220

Closed
wants to merge 20 commits into from

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Jan 9, 2015

Closes #6295

@@ -37,8 +37,6 @@
*/
public class NodesInfoResponse extends NodesOperationResponse<NodeInfo> implements ToXContent {

private SettingsFilter settingsFilter;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this was removed?

Changes after the first round of review by @bleskes
@@ -55,7 +55,7 @@ public ClusterName getClusterName() {
public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
clusterName = ClusterName.readClusterName(in);
clusterState = ClusterState.Builder.readFrom(in, null, clusterName);
clusterState = ClusterState.Builder.readFrom(in, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's more intuitive to have a readFrom(StreamInput input) that delegates than passing null here?

@s1monw
Copy link
Contributor

s1monw commented Feb 9, 2015

I went over this briefly and I am surprised how brief it is compared to what I expected. Yet, I am kind of overwhelmed by the complexity it adds compared to the previous way of doing it. I personally think this is mainly due to the large hierarchies that are added here like we have tons of impl of this ClusterStatePart interface that I really feel can be much more simplified. I also think we have to work on the inner Factory classes and trie to prevent them altogether. It make the change really hard to understand IMO.

I know this is not an easy change but I think we need to really watch out to not add too much complexity here.

@imotov
Copy link
Contributor Author

imotov commented Feb 18, 2015

@s1monw @bleskes I am going to close this PR and split it into a series of smaller RPs to facilitate more efficient review process. First installment is #9748.

@imotov imotov closed this Feb 18, 2015
@bleskes
Copy link
Contributor

bleskes commented Feb 18, 2015

++

On 18 Feb 2015, at 16:48, Igor Motov notifications@github.com wrote:

@s1monw @bleskes I am going to close this PR and split it into a series of smaller RPs to facilitate more efficient review process. First installment is #9748.


Reply to this email directly or view it on GitHub.

@imotov imotov deleted the issue-6295-diff-state branch May 1, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Core Core issues without another label >enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only send diff of cluster state instead of full cluster state
4 participants