[elasticsearch,elasticsearch5] Add Elasticsearch 5.x binding by risdenk · Pull Request #925 · brianfrankcooper/YCSB

risdenk · 2017-02-07T02:41:00Z

Fixes #893

Elasticsearch 5.x Highlights:

Option for Elasticsearch 5.x Transport or REST client
scan is changed to not implemented (due to not being about to range query over _id or _uid)
skip tests on JDK 7

Other changes:

Cleaned up Elasticsearch 2.x test for Java 7+
Exposed elasticsearch version in top level pom.xml
Reordered properties in top level pom.xml alphabetically

risdenk · 2017-02-07T02:41:52Z

@jasontedor I know that ES is moving towards the Java REST client but this was the closest to what the existing Elasticsearch binding was doing. If its not the right approach no worries.

jasontedor · 2017-02-07T03:34:35Z

Thanks a lot for doing thing @risdenk.

Yeah, I was going to use the low-level REST client when I made time to address #893.

Also, my plan was to support scan by indexing another field, and it'd probably be better to let auto-IDs be employed here.

Do you want to make those changes or would you like me to? (To be clear, I'm happy either way.)

risdenk · 2017-02-07T15:07:37Z

Your ideas sounds good to me. I might take a crack at it tonight. If I don't get to it feel free to work on it.

risdenk · 2017-02-08T03:26:01Z

@jasontedor This is still a work in progress but pushed my changes just in case its helpful.

I started working on the REST client here: ElasticsearchRestClient.java

Its not integrated yet with the tests or as far as the binding goes. Just a start to see how it would lay out. I'm hoping we can opt for either Transport or REST client to allow benchmarking both.

MichaelTong · 2017-06-22T17:50:33Z

Hi @risdenk , I checked out your branch and trying to build the elasticsearch5 client. After I put a "elasticsearch5" entry in bin/ycsb and started to run it, it threw exception saying IllegalArgumentException.

Exception in thread "Thread-3" java.lang.IllegalArgumentException: unknown setting [access] please check that any required plugins are installed, or check the bre
aking changes documentation for removed settings
        at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:276)
        at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:244)
        at org.elasticsearch.common.settings.SettingsModule.<init>(SettingsModule.java:138)
        at org.elasticsearch.client.transport.TransportClient.buildTemplate(TransportClient.java:138)
        at org.elasticsearch.client.transport.TransportClient.<init>(TransportClient.java:258)
        at org.elasticsearch.transport.client.PreBuiltTransportClient.<init>(PreBuiltTransportClient.java:125)
        at org.elasticsearch.transport.client.PreBuiltTransportClient.<init>(PreBuiltTransportClient.java:111)
        at org.elasticsearch.transport.client.PreBuiltTransportClient.<init>(PreBuiltTransportClient.java:101)
        at com.yahoo.ycsb.db.elasticsearch5.ElasticsearchClient.init(ElasticsearchClient.java:113)
        at com.yahoo.ycsb.DBWrapper.init(DBWrapper.java:85)
        at com.yahoo.ycsb.ClientThread.run(Client.java:415)
        at java.lang.Thread.run(Thread.java:748)
        Suppressed: java.lang.IllegalArgumentException: unknown setting [db] please check that any required plugins are installed, or check the breaking changes d
ocumentation for removed settings
                ... 12 more
        Suppressed: java.lang.IllegalArgumentException: unknown setting [dotransactions] please check that any required plugins are installed, or check the breaki
ng changes documentation for removed settings
                ... 12 more
        Suppressed: java.lang.IllegalArgumentException: unknown setting [es.hosts.list] please check that any required plugins are installed, or check the breakin
g changes documentation for removed settings
                ... 12 more
        Suppressed: java.lang.IllegalArgumentException: unknown setting [es.remote] please check that any required plugins are installed, or check the breaking ch
anges documentation for removed settings
                ... 12 more
        Suppressed: java.lang.IllegalArgumentException: unknown setting [fieldcount] please check that any required plugins are installed, or check the breaking c
hanges documentation for removed settings
                ... 12 more
        Suppressed: java.lang.IllegalArgumentException: unknown setting [fieldlength] please check that any required plugins are installed, or check the breaking
changes documentation for removed settings
                ... 12 more

It seems that it also put workload file into setting

levaphenyl · 2017-07-11T15:26:53Z

Hello,

Same problem than @MichaelTong here. Apparently, when choosing es.remote=true, all parameters passed in the command line or in the workload file are dumped in the variable setting, including db, threadcount etc.

The second bug is that the client throws a NullPointerException if path.home is not given even if es.remote=true.

I just added a pull request to fix this.

Hope this helps :).

manolama · 2017-08-05T18:56:18Z

@risdenk Should we still target this for 0.13? Thanks!

jasontedor · 2017-08-06T00:29:09Z

@manolama See my comment on #981.

* master: [core] Fixing squid:S1319 - Declarations should use Java collection interfaces such as "List" rather than specific implementation classes such as "LinkedList". (manolama - updated bindings added since the PR) [core] Use longs instead of ints to support larger key spaces. Changed int to long in Measurements code to support large scale workloads. (manolama - fixed checkstyle errors) [core] Export totalHistogram for HdrHistogram measurement [core] Add an operation enum to the Workload class. This can eventually be used to replace the strings. [core] Add a Fisher-Yates array shuffle to the Utils class. [core] Fix an issue where the threadid and threadCount were not passed to the workload client threads. Had to use setters to get around the checkstyle complaint of having too many parameters. Upgrading googlebigtable to the latest version. The API used by googlebigtable has had quite a bit of churn. This is the minimal set of changes required for the upgrade. [geode] Update to apache-geode 1.2.0 release [core] Update to use newer version of Google Cloud Spanner client and associated required change [core] Add a reset() method to the ByteIterator abstract and implementations for each of the children. This lets us re-use byte iterators if we need to access the values again (when applicable). [hbase12] Add HBase 1.2+ specific client that relies on the shaded client artifact provided by those versions. (brianfrankcooper#970) [distro] Refresh Apache licence text (brianfrankcooper#969) [memcached] support binary protocol (brianfrankcooper#965) [accumulo] A general "refresh" to the Accumulo binding (brianfrankcooper#947) [cloudspanner] Add binding for Google's Cloud Spanner. (brianfrankcooper#939) [aerospike] Change the write policy to REPLACE_ONLY (brianfrankcooper#937)

This commit bumps the Elasticsearch dependency version of the Elasticsearch 5 binding to version 5.5.1.

If path.home is not set we should not add it to the settings object used to construct the transport client otherwise we will hit a null pointer exception due to the null value being dereferenced in the internals of Elasticsearch.

Since Elasticsearch 5, Elasticsearch is now strict about settings. This means that if you pass it a setting that it does not recognize, Elasticsearch will throw an exception whereas previously it was lenient in such situations. This commit removes passing all properties as settings to Elasticsearch in favor of a special prefix es.setting for which properties prefixed with this will be passed as settings to Elasticsearch.

Upstream Elasticsearch does not support embedded nodes as of Elasticsearch 5. This commit removes the usage of embedded nodes within YCSB.

This commit refactors the indexing of documents in the Elasticsearch 5 binding to use auto-generated IDs, instead indexing the key field as a dedicated field rather than using it as the ID. This enables us to implement scan functionality which we add in this commit as well.

We can remove the path.home setting for the Elasticsearch 5 binding for the transport client so this commit does that.

This commit fixes a bug in the Elasticsearch 5 transport client binding. Namely, the update method was not indexing using the ID of the read document, but instead the provided key. As auto-generated IDs are used, this is a mistake as this is not the ID of the document. This commit fixes this issue. Additionally, the type is passed to the search method and used in searches. While this is not a correctness issue per se, we fix it for clarity.

This commit is an initial cut at a complete implementation using the low-level Elasticsearch REST client.

This commit is a straightforward code cleanup of the Elasticsearch 5 transport client and REST client implementations.

This commit fixes issues with the Elasticsearch 5 transport client and REST client bindings so that all tests pass.

This commit updates the Elasticsearch 5 binding docs to include references for starting Elasticsearch 5, how to configure the transport client, and how to use the low-level REST client binding.

jasontedor · 2017-08-09T05:27:34Z

@risdenk I think that this a ready now as tests are passing now for both the transport client and REST client bindings. I'm not sure who should review this, can you?

This commit sets the Elasticsearch 5 binding to only use the elasticsearch-maven-plugin if tests are running on JDK 8.

levaphenyl · 2017-08-09T07:10:32Z

@jasontedor The current implementation for the ES5 binding does not support scan operations. In ES5, RangeQueries are not allowed on the field _id anymore. I proposed a implementation using matchQuery in my previous pull request in risdenk/YCSB but I'm not sure since I am neither a Java developer, nor a ES expert.

jasontedor · 2017-08-09T07:13:54Z

@levaphenyl I updated this pull request to add support for scan by indexing another field that we can do range queries on. I made almost no effort to optimize this, only to get something working first that can be iterated on later after integration (first make it right, then make it fast). 😄

levaphenyl · 2017-08-09T07:19:32Z

@jasontedor Sorry, I didn't read your commits well enough. This looks nice and adding the support for auto-ID is a great idea! Thank you 😄.

jasontedor · 2017-08-09T07:25:11Z

@levaphenyl You're welcome, thanks for your interest!

This commit adds some basic formatting to the transport client logs for the transport client used in the Elasticsearch 5 binding.

risdenk · 2017-08-10T01:40:48Z

@jasontedor - I'll take a look

risdenk

@jasontedor Changes look good. I didn't get a chance to run it yet probably tomorrow.

Most of the comments are around stderr vs stdout. Stdout is typically reserved for metric output only and everything else to stderr. The other thought is using log4j instead of System...println/err.printStackTrace() for some of the messages.

risdenk · 2017-08-10T01:44:10Z

+      }
+    }
+    final String clusterName = settings.get("cluster.name");
+    System.out.println("Elasticsearch cluster name = " + clusterName);


Think this should go to System.err if we need it. Metrics should go to standard out.

I dropped it: e30441c

risdenk · 2017-08-10T01:44:17Z

+            .put("client.transport.nodes_sampler_interval", "30s");
+    // Default it to localhost:9300
+    final String[] nodeList = props.getProperty("es.hosts.list", DEFAULT_REMOTE_HOST).split(",");
+    System.out.println("Elasticsearch Remote Hosts = " + props.getProperty("es.hosts.list", DEFAULT_REMOTE_HOST));


Think this should go to System.err if we need it. Metrics should go to standard out.

I dropped it: e30441c

risdenk · 2017-08-10T01:45:08Z

+    es.index.key=es.ycsb
+    es.number_of_shards=1
+    es.number_of_replicas=0
+    es.newdb=false


Does newdb make sense here? New index?

I pushed d791ae6.

risdenk · 2017-08-10T01:46:40Z

+
+      return Status.OK;
+    } catch (final Exception e) {
+      e.printStackTrace();


I think this should go to stderr. Not sure it does by default.

It does, Exception#printStackTrace goes to stderr by default.

risdenk · 2017-08-10T01:46:51Z

+
+      return Status.OK;
+    } catch (final Exception e) {
+      e.printStackTrace();


should go to stderr

Exception#printStackTrace goes to stderr by default.

risdenk · 2017-08-10T01:50:08Z

+      }
+      return Status.OK;
+    } catch (final Exception e) {
+      e.printStackTrace();


Exception#printStackTrace goes to stderr by default.

risdenk · 2017-08-10T01:52:45Z

@@ -0,0 +1,7 @@
+appender.console.type = Console


Not sure what is getting logged by this but if it is during the YCSB testing should go to stderr instead of stdout.

It's for any internal logging produced by the transport client (for example, if there are issues connecting to the Elasticsearch cluster, a user can set es.setting.logger.org.elasticsearch to debug and see the debugging logging by the transport client to help debug such an issue. I pushed a65dcb7 so that these logs go to stderr.

risdenk · 2017-08-10T01:53:31Z

      <groupId>org.apache.thrift</groupId>
      <artifactId>libthrift</artifactId>
-      <version>${thrift.version}</version>
+      <version>0.8.0</version>


Why change variable here?

Hmm...not sure where that came from, was not intentional and will revert.

I pushed 6ae7050.

risdenk · 2017-08-10T01:53:44Z

      <groupId>org.apache.openjpa</groupId>
      <artifactId>openjpa-jdbc</artifactId>
-      <version>${openjpa.jdbc.version}</version>
+      <version>2.1.1</version>


Why change variable here?

Hmm...not sure where that came from, was not intentional and will revert.

I pushed 6ae7050.

risdenk · 2017-08-10T01:54:16Z

    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
    <thrift.version>0.8.0</thrift.version>
    <hypertable.version>0.9.5.6</hypertable.version>
+    <elasticsearch-version>2.4.4</elasticsearch-version>                                                                                                                                      


elasticsearch-version not used anywhere?

I pushed 2250903.

This commit reverts some unrelated changes to POMs for other bindings, perhaps made inadvertently while merging master into a development branch.

This commit sets the log of the internal transport client used in the Elasitcsearch 5 binding to use standard error instead of standard out.

This commit removes some unnecessary logging statements for the elasticsearch5 and elasticsearch5-rest bindings.

This commit changes the name of a property, es.newdb, to es.new_index as this is more consistent with the terminology used in Elasticsearch.

This commit wraps the usage of a content builder in a try-with-resources block to ensure that it is correctly closed.

This commit removes an unrelated POM change for the original (Elasticsearch 2) binding.

The comments were incorrectly referring to ElasticsearchClient but they should be referring to ElasticsearchRestClient instead.

jasontedor · 2017-08-10T02:34:23Z

Thanks @risdenk, I've responded to your feedback.

risdenk · 2017-08-10T02:37:28Z

@jasontedor I see your referenced commits but they aren't on this PR like the other commits? Where did they end up? Its weird that I can't seem to find them but the links you posted work.

jasontedor · 2017-08-10T02:51:01Z

I'm not sure, I agree they are not visible on the PR. I also noticed that I did not receive email notifications for the pushes.

jasontedor · 2017-08-10T02:51:42Z

@risdenk I think they are visible now.

risdenk

Changes look good. Tests run as well. Few minor comments from opening in an IDE.

ElasticsearchClientIT and ElasticsearchRestClientIT are basically identical
- Could be useful to make one test?
ElasticsearchClient and ElasticsearchRestClient have duplicated methods
- Might be worth it to consolidate?
Since org.apache.http is already included could use HttpStatus.SC_CREATED for 201 and others (404, 200, etc).

No other major things from me so could be merged.

risdenk · 2017-08-10T03:06:51Z

+    return performRequest(restClient, method, endpoint, params, null);
+  }
+
+  private static Header[] emptyHeaders = new Header[0];


nit: could be final?

I pushed 38d45b0.

This commit removes duplicate test code from the elasticsearch5 and elasticsearch5-rest test classes.

This commit marks a field intended to be treated as a constant as final.

This commit replaces some magic numbers (HTTP status codes) with already-existing constants for this purpose.

This commit removes an extraneous newline from ElasticsearchRestClient.java.

jasontedor · 2017-08-10T07:35:07Z

Thanks @risdenk! Regarding your last few suggestions I addressed them all except for consolidating code between ElasticsearchClient and ElasticsearchRestClient, I prefer keeping the refresh logic separate.

levaphenyl · 2017-08-28T16:25:41Z

Just tested the implementation on my cluster and I get a NoNodeAvailableException for 6 of 48 threads with the elasticsearch5 binding. This problem doesn't appear with the elasticsearch5-rest binding.

risdenk added db-new elasticsearch labels Feb 7, 2017

risdenk added this to the 0.13.0 milestone Feb 7, 2017

risdenk requested a review from jasontedor February 7, 2017 02:41

risdenk force-pushed the initial-es5 branch from 2e68825 to 534a845 Compare February 8, 2017 01:29

[elasticsearch,elasticsearch5] Add Elasticsearch 5.x binding

c3cfc98

risdenk force-pushed the initial-es5 branch from 534a845 to c3cfc98 Compare February 8, 2017 03:23

Start REST integration tests

c52c438

levaphenyl mentioned this pull request Jul 11, 2017

Elasticsearch 5.x.x binding: Avoiding IllegalArgumentException risdenk/YCSB#1

Open

jasontedor mentioned this pull request Aug 5, 2017

Release 0.13.0 #981

Closed

jasontedor added 12 commits August 7, 2017 08:46

Elasticsearch 5: Set dependency to version 5.5.1

b1e1d48

This commit bumps the Elasticsearch dependency version of the Elasticsearch 5 binding to version 5.5.1.

Elasticsearch 5: Avoid adding path.home if not set

a3ba64d

If path.home is not set we should not add it to the settings object used to construct the transport client otherwise we will hit a null pointer exception due to the null value being dereferenced in the internals of Elasticsearch.

Elasticsearch 5: Remove support for embedded node

d66e856

Upstream Elasticsearch does not support embedded nodes as of Elasticsearch 5. This commit removes the usage of embedded nodes within YCSB.

Elasticsearch 5: Remove path.home setting

db8674a

We can remove the path.home setting for the Elasticsearch 5 binding for the transport client so this commit does that.

Elasticsearch 5: Complete REST implementation

bc69e7e

This commit is an initial cut at a complete implementation using the low-level Elasticsearch REST client.

Elasticsearch 5: Code cleanup

f1eed61

This commit is a straightforward code cleanup of the Elasticsearch 5 transport client and REST client implementations.

Elasticsearch 5: Fix issues for tests to pass

42bb148

This commit fixes issues with the Elasticsearch 5 transport client and REST client bindings so that all tests pass.

Elasticsearch 5: Update docs

382bdc7

This commit updates the Elasticsearch 5 binding docs to include references for starting Elasticsearch 5, how to configure the transport client, and how to use the low-level REST client binding.

jasontedor removed their request for review August 9, 2017 05:27

Elasticsearch 5: Only activate plugin on JDK 8

6d14ff4

This commit sets the Elasticsearch 5 binding to only use the elasticsearch-maven-plugin if tests are running on JDK 8.

Elasticsearch 5: Format transport client logs

1774d62

This commit adds some basic formatting to the transport client logs for the transport client used in the Elasticsearch 5 binding.

risdenk commented Aug 10, 2017

View reviewed changes

jasontedor added 7 commits August 10, 2017 11:08

Elasticsearch 5: Revert unrelated changes

6ae7050

This commit reverts some unrelated changes to POMs for other bindings, perhaps made inadvertently while merging master into a development branch.

Elasticsearch 5: Log transport client on stderr

a65dcb7

This commit sets the log of the internal transport client used in the Elasitcsearch 5 binding to use standard error instead of standard out.

Elasticsearch 5: Remove unneeded output statements

e30441c

This commit removes some unnecessary logging statements for the elasticsearch5 and elasticsearch5-rest bindings.

Elasticsearch 5: Change es.newdb to es.new_index

d791ae6

This commit changes the name of a property, es.newdb, to es.new_index as this is more consistent with the terminology used in Elasticsearch.

Elasticsearch 5: Close content builder

9267038

This commit wraps the usage of a content builder in a try-with-resources block to ensure that it is correctly closed.

Elasticsearch 5: Remove core POM change

2250903

This commit removes an unrelated POM change for the original (Elasticsearch 2) binding.

Elasticsearch 5: Fix Javadocs in REST client tests

5d840ef

The comments were incorrectly referring to ElasticsearchClient but they should be referring to ElasticsearchRestClient instead.

risdenk commented Aug 10, 2017

View reviewed changes

jasontedor added 4 commits August 10, 2017 16:20

Elasticsearch 5: Collapse tests to one class

b9f8539

This commit removes duplicate test code from the elasticsearch5 and elasticsearch5-rest test classes.

Elasticsearch 5: Mark constant as final

38d45b0

This commit marks a field intended to be treated as a constant as final.

Elasticsearch 5: Use provided constants for status codes

082dc30

This commit replaces some magic numbers (HTTP status codes) with already-existing constants for this purpose.

Elasticsearch 5: Remove extraneous newline

033ff88

This commit removes an extraneous newline from ElasticsearchRestClient.java.

jasontedor merged commit 385a389 into brianfrankcooper:master Aug 10, 2017

busbey mentioned this pull request May 19, 2018

Release 0.14.0 #1117

Closed

Conversation

risdenk commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risdenk commented Feb 7, 2017

Uh oh!

jasontedor commented Feb 7, 2017

Uh oh!

risdenk commented Feb 7, 2017

Uh oh!

risdenk commented Feb 8, 2017

Uh oh!

MichaelTong commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levaphenyl commented Jul 11, 2017

Uh oh!

manolama commented Aug 5, 2017

Uh oh!

jasontedor commented Aug 6, 2017

Uh oh!

jasontedor commented Aug 9, 2017

Uh oh!

levaphenyl commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Aug 9, 2017

Uh oh!

levaphenyl commented Aug 9, 2017

Uh oh!

jasontedor commented Aug 9, 2017

Uh oh!

risdenk commented Aug 10, 2017

Uh oh!

risdenk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

risdenk commented Feb 7, 2017 •

edited

Loading

MichaelTong commented Jun 22, 2017 •

edited

Loading

levaphenyl commented Aug 9, 2017 •

edited

Loading

jasontedor Aug 10, 2017 •

edited

Loading

levaphenyl commented Aug 28, 2017 •

edited

Loading