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

revamp TransportRequest handlers to support Writeable #26315

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Aug 22, 2017

This PR begins the long journey to deprecating Streamable.

The idea here is to add additional method signatures that
support Writeable.Reader, so that the work to migrate objects TransportMessage to
implement Writeable and not Streamable.

One example conversion is done in this PR: SimulatePipelineRequest.

@talevy talevy added :Core/Infra/Core Core issues without another label >non-issue review WIP labels Aug 22, 2017
@talevy talevy requested a review from nik9000 August 22, 2017 00:44
@talevy talevy removed the WIP label Aug 22, 2017
@talevy talevy force-pushed the allow-writeable-readers-as-suppliers branch from 638fbef to 43fd46a Compare August 22, 2017 00:50
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I left some comments. Looks good in general.

@@ -41,7 +42,7 @@

import static org.elasticsearch.ingest.IngestDocument.MetaData;

public class SimulatePipelineRequest extends ActionRequest {
public class SimulatePipelineRequest extends ActionRequest implements Writeable {

private String id;
Copy link
Member

Choose a reason for hiding this comment

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

These can all become final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think not, because of the client usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if the transport-client request builder pattern is to be deprecated, I think it may be best for that to be done in its own context in separate PRs

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, I could not see all the setters in the review without expanding.


private TransportAddress remoteAddress;

public TransportMessage() {
Copy link
Member

Choose a reason for hiding this comment

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

a single, explicit empty ctor should not be necessary (that is what the default ctor is)

@@ -39,6 +39,11 @@
public TransportRequest() {
}

public TransportRequest(StreamInput in) throws IOException {
super();
Copy link
Member

Choose a reason for hiding this comment

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

explicit call to default ctor is not necessary

public void testUnsupportedReadFrom() {
SimulatePipelineRequest request = new SimulatePipelineRequest();
Exception exception = expectThrows(UnsupportedOperationException.class,() -> request.readFrom(null));
assertThat(exception.getMessage(), equalTo("usage of Streamable is to be replaced by Writeable"));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be adding a test for each one of these. It is not part of our API (that these are returning UOE).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK cool. I was on the fence about it. figured I'd discuss it after the work has been done :) will happily remove

@talevy talevy force-pushed the allow-writeable-readers-as-suppliers branch 2 times, most recently from 1fa601e to 51885b3 Compare August 22, 2017 04:28
This PR begins the long journey to deprecating Streamable.

The idea here is to add additional method signatures that
support Writeable.Reader, so that the work to migrate objects TransportMessage to
implement Writeable and not Streamable.

One example conversion is done in this PR: SimulatePipelineRequest.
@talevy talevy force-pushed the allow-writeable-readers-as-suppliers branch from 51885b3 to ccf5717 Compare August 22, 2017 16:31
@talevy
Copy link
Contributor Author

talevy commented Aug 22, 2017

thanks, updated with changes reflecting the review

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@talevy talevy merged commit 6ab4b6b into elastic:master Aug 22, 2017
@talevy talevy deleted the allow-writeable-readers-as-suppliers branch August 22, 2017 22:47
@nik9000
Copy link
Member

nik9000 commented Aug 23, 2017

Awesome! I'm sorry I missed this yesterday. I'm excited to see the start.

@dnhatn
Copy link
Member

dnhatn commented Jan 12, 2018

@talevy Is it possible to backport this to 6.x?

@talevy
Copy link
Contributor Author

talevy commented Jan 16, 2018

I suppose, I guess I didn't think about it since it was a work-in-progress transitional interface. If you think I should, I'd be happy to. I can imagine one would want to leverage the writeable interface in new actions in 7.0 that need to be backported to 6.x, so these aspects of the constructor could potentially cause merging issues? Is this what you are referring to?

@dnhatn
Copy link
Member

dnhatn commented Jan 16, 2018

@talevy. Yes, this actually happened to me. I was using a new API in V7 but had to use the old one for 6.x.

ywelsch added a commit that referenced this pull request Jan 29, 2018
Backport of PR to 6.x, excluding the changes to SimulatePipelineRequest
ywelsch added a commit that referenced this pull request Jan 29, 2018
Backport of PR to 6.x, excluding the changes to SimulatePipelineRequest
@ywelsch
Copy link
Contributor

ywelsch commented Jan 29, 2018

I've backported this PR (minus changes to SimulatePipelineRequest) to 6.x and 6.2 (39f62de and 9df8da4), as backporting other PRs turned out to be difficult without this.

nik9000 pushed a commit that referenced this pull request Jul 31, 2018
-- This is a pre-stage for adding the reindex API to the REST high-level-client
-- Follows the pattern set in #26315
nik9000 pushed a commit that referenced this pull request Jul 31, 2018
-- This is a pre-stage for adding the reindex API to the REST high-level-client
-- Follows the pattern set in #26315
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.

6 participants