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

Add REST high level client gradle submodule and first simple method #22371

Merged
merged 4 commits into from Jan 5, 2017

Conversation

Projects
None yet
4 participants
@javanna
Copy link
Member

commented Dec 29, 2016

The RestHighLevelClient class takes as as an argument a low level client instance (RestClient). The first method added is ping, which returns true if the call to HEAD / went ok and false if an IOException was thrown. Any other exception gets bubbled up.

There are two kinds of tests, a unit test (RestHighLevelClientTests) that verifies the interaction between high level and low level client, and an integration test (MainActionIT) which relies on an externally started es cluster to send requests to.

This PR essentially holds the current content of the feature/high-level-rest-client public branch. We decided to add parsing code to java api responses directly upstream. Also we can add new methods to the high level rest client as we go without the need to maintain a separate branch. The important part is that we don't release/publish any artifact until we are ready to do so.

@javanna

This comment has been minimized.

Copy link
Member Author

commented Dec 30, 2016

@elasticmachine retest this please

@javanna

This comment has been minimized.

Copy link
Member Author

commented Dec 30, 2016

@nik9000 can you check out if the new gradle submodule works ok with eclipse please? I suspect it doesn't, in which case, do you know what we may have forgotten in the gradle conf?

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2016

In Eclipse this isn't importing the "main" classes, only the test classes. this fixes it, but it isn't super clear to me why we haven't hit this before. If we have, we work around it in some way I haven't yet seen.

* under the License.
*/
apply plugin: 'elasticsearch.build'
apply plugin: 'elasticsearch.rest-test'

This comment has been minimized.

Copy link
@rjernst

rjernst Dec 30, 2016

Member

This is meant to be for a standalone test, not adding a rest test to an existing project. It's possible your changes to standalone test make this work, but it seems a little wonky to me. This may be the reason for the eclipse behavior Nik described.

This comment has been minimized.

Copy link
@javanna

javanna Dec 30, 2016

Author Member

ok I don't know anything about these gradle changes, I just took what we had in the feature branch, I thought that you and Tanguy had already worked together on it. Could you let me know what else I should be doing instead please?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jan 2, 2017

Member

Not knowing much about gradle and the build setup really, but in my initial commit in the feature branch I only used the elasticsearch.build plugin and added the integTest task manually (ecf6af8) but this very likely wasn't the best approach either.

This comment has been minimized.

Copy link
@javanna

javanna Jan 3, 2017

Author Member

This is the only comment I haven't addressed yet, as I am not clear on what should be done.

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 3, 2017

Contributor

I think the trouble is https://github.com/elastic/elasticsearch/blob/master/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/StandaloneTestBasePlugin.groovy#L46-L49

I think if we want to repurpose RestTestPlugin so it can be applied to projects with a main class we should do something like:

- project.pluginManager.apply(StandaloneTestBasePlugin)
+ if (false == project.pluginManager.hasPlugin('elasticsearch.build')) {
+   project.pluginManager.apply(StandaloneTestBasePlugin)
+ }

Or maybe just:

- project.pluginManager.apply(StandaloneTestBasePlugin)
+ if (false == project.pluginManager.hasPlugin('elasticsearch.build') && false == project.pluginManager.hasPlugin('elasticsearch.standalone-test')) {
+   throw new InvalidUserDataException('elasticsearch.rest-test requires either elasticsearch.build or elasticsearch.standalone-test')
+ }

This comment has been minimized.

Copy link
@javanna

javanna Jan 3, 2017

Author Member

@rjernst are you ok with what Nik is proposing?

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 3, 2017

Contributor

If it sounds good I'm happy to make the change and add a commit to this PR.

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 3, 2017

Member

I'm fine with what Nik proposes. It would mean the tests we have under qa right now need to add apply plugin: 'elasticsearch.standalone-test'.

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 3, 2017

Contributor

Yeah, that is the plan. I'll have a look at it soon!

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 3, 2017

Contributor

I pushed a commit that does the bottom option: now elasticsearch.rest-test requires either elasticsearch.build or elasticsearch.standalone-test.

if (arrayName != null && rarely()) {
headerName = arrayName;
protected static void assertHeaders(final Header[] defaultHeaders, final Header[] requestHeaders,
final Header[] actualHeaders, final Set<String> ignoreHeaders) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jan 2, 2017

Member

Nit: can this helper method get some comment about what the arguments mean? They are named appropriately I think but some short explanation would make reuse easier.

client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java Outdated
@@ -81,4 +85,22 @@ static int randomErrorRetryStatusCode(Random random) {
static List<Integer> getAllStatusCodes() {
return ALL_STATUS_CODES;
}

/**
* Create the specified number of {@link Header}s.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jan 2, 2017

Member

Do we want to be able to specify the number as the "generate" method was able before? If not the comment should be changed.

client/test/src/main/java/org/elasticsearch/client/RestClientTestUtil.java Outdated
/**
* Create the specified number of {@link Header}s.
* <p>
* Generated header names will be the {@code baseName} plus its index or, rarely, the {@code arrayName} if it's supplied.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jan 2, 2017

Member

Nit: arrayName is not used here any more, not sure if we need it, if not remove from comment.

@@ -76,8 +74,7 @@ public static void startHttpServer() throws Exception {
}

httpServer = createHttpServer();
int numHeaders = randomIntBetween(0, 5);
defaultHeaders = generateHeaders("Header-default", "Header-array", numHeaders);
defaultHeaders = RestClientTestUtil.randomHeaders(getRandom(), "Header-default");

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jan 2, 2017

Member

Also mentioned below, I don't know which purpose array name ("Header-array") served here before, just curious to see why its gone (unnecessary?).

* under the License.
*/
apply plugin: 'elasticsearch.build'
apply plugin: 'elasticsearch.rest-test'

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jan 2, 2017

Member

Not knowing much about gradle and the build setup really, but in my initial commit in the feature branch I only used the elasticsearch.build plugin and added the integTest task manually (ecf6af8) but this very likely wasn't the best approach either.

@javanna javanna force-pushed the javanna:enhancement/rest_high_level_client branch Jan 3, 2017

@javanna

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2017

@cbuescher thanks for the review, your comments were all spot-on, I addressed them with the last commit

@javanna javanna force-pushed the javanna:enhancement/rest_high_level_client branch Jan 4, 2017

@javanna

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2017

retest this please

@nik9000 nik9000 force-pushed the javanna:enhancement/rest_high_level_client branch Jan 5, 2017

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2017

@javanna I think I fixed it.

javanna and others added some commits Dec 29, 2016

[TEST] move randomHeaders method from RestClientTestCase to RestClien…
…tTestUtil and simplify headers assertions
add REST high level client gradle submodule and first simple method
The RestHighLevelClient class takes as as an argument a low level client instance RestClient. The first method added is ping, which returns true if the call to HEAD / went ok and false if an IOException was thrown. Any other exception gets bubbled up.

There are two kinds of tests, a unit test (RestHighLevelClientTests) that verifies the interaction between high level and low level client, and an integration test (MainActionIT) which relies on an externally started es cluster to send requests to.
Require either BuildPlugin or StandaloneTestBasePlugin to use RestTes…
…tPlugin

It used to be that RestTestPlugin "came with" StandaloneTestBasePlugin
but we'd like to use it with BuildPlugin for the high level rest client.
Switch from standalone-test to standalone-rest-test
standalone-rest-test doesn't configure unit tests and for these
integ test only tests, that is what we want.
@javanna

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2017

@nik9000 thanks a lot, all good now, going to push.

@javanna javanna force-pushed the javanna:enhancement/rest_high_level_client branch to 326f6ca Jan 5, 2017

@javanna javanna merged commit 232af51 into elastic:master Jan 5, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author has signed the CLA
Details
@javanna

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2017

I am wondering if we should backport the gradle changes, not the the new submodule for now, to 5.x. That will make backporting the submodule easier when ready. Thoughts @nik9000 ?

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.