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

migrate some MasterNodeRequest subclasses to Writeable Readers #26463

Merged
merged 2 commits into from
Sep 1, 2017

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Aug 31, 2017

migrating more classes to implement Writeable.Reader functional interfaces
and deprecate Streamable's readFrom.

The main goal for this PR is to help transition the MasterNodeRequest subclassed request objects. Since there are more than I'd like to count, more will follow in another PR.

AcknowledgedRequests will be next

@talevy talevy added :Core/Infra/Core Core issues without another label >non-issue v7.0.0 labels Aug 31, 2017
@talevy talevy requested a review from nik9000 August 31, 2017 22:54
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Other than the leftover I think this is good.

@@ -693,6 +694,9 @@ public static void assertVersionSerializable(Version version, Streamable streama
// and the readFrom method throws an exception if called
Streamable newInstanceFromStream = tryCreateFromStream(streamable, input);
if (newInstanceFromStream == null) {
if (newInstance instanceof ClusterAllocationExplainRequest) {
System.out.println("what?");
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a leftover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh me oh my. indeed. I was debugging. will remove. thanks!

Copy link
Member

Choose a reason for hiding this comment

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

If you add // nocommit any time that you add a debug line like this, the build will help you remember to remove it (a pre-commit check will fail).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the protip! I will

@talevy talevy force-pushed the masternodewriteable branch from 2bf37df to 4821aeb Compare September 1, 2017 17:54
@talevy talevy force-pushed the masternodewriteable branch from 4821aeb to 6db927e Compare September 1, 2017 17:55
@talevy
Copy link
Contributor Author

talevy commented Sep 1, 2017

merging even though build failed. Build failed due to bwc tests thinking 6.0-beta2 is the current 6.0 build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants