Skip to content
This repository was archived by the owner on Jun 29, 2021. It is now read-only.

Jest (Rest) based ElasticSearch implementation#67

Closed
LosD wants to merge 16 commits intoapache:masterfrom
LosD:master
Closed

Jest (Rest) based ElasticSearch implementation#67
LosD wants to merge 16 commits intoapache:masterfrom
LosD:master

Conversation

@LosD
Copy link
Copy Markdown
Contributor

@LosD LosD commented Oct 28, 2015

After looking at updating ElasticSearch 2.0, I got a bit fed up with the compatibility issues marring our current ElasticSearch implementation.

Here is a suggestion for a REST-based ElasticSearch DataContext using Jest. It is still dependent on ElasticSearch, as it uses SearchSourceBuilder for generating the queries. This also means that we're not completely independent from ES version, but the problem is much more limited now, and run time replacement of the Jest library version should be possible. Until a working version of Jest 2.0.0 (The current SNAPSHOT seems to have some problems with HTTPClient and AsyncHttpClient compatibility), we can't really verify that, though.

Over time, we should be able to get rid of SearchSourceBuilder by generating the JSON manually.

Testing is still directly dependent on the ES version, as it is using the ES node client.

I didn't find a simple way to get the HTTP port from the node client, 9201 is currently assumed.

A worry is the workaround in JestElasticSearchUtils.getDataFromColumnType(). I'd love to find a better way. The issue is that GSON uses LazilyParsedNumber for all numbers, which is not recognized as a Number (by either Jest or GSON) when feeding back into Jest for updates.

Dennis Du Krøger and others added 9 commits October 23, 2015 16:26
Nowhere close to being done
 ElasticSearchDataContext compiles now (no guaranteed, of course), the directly supporting cast as well.
All querying seems to work now, except for group by. However, that
works if standalone, or only run with the other query tests, so I'm
pretty sure that is because a failure in the
drop/create/update/etf tests leaves the ES in a bad state.
I'm a bit unsure about the workaround for LazilyParsedNumber. What
about BitDecimal and BigInteger?
Avoids confusion by prefixing all Jest-based classes with Jest. Also
makes Jest ES available in DataContextFactory.
Makes system more independent from ES version
@kaspersorensen
Copy link
Copy Markdown
Contributor

I gave this one a quick scan through, and for me it's looking quite OK. I have some remarks:

  • The build is breaking on Travis. This should be fixed one way or the other.
  • It's a pity about the dependency still on elasticsearch. But not because of the dependency itself I think, but because it still complicates the code when you build query filter etc. and have to use reflection there. This would be a lot better to build dynamically using a Json request builder of sorts. For me this is not a blocker, but something we should probably make sure to report in JIRA as an improvement we would like to make eventually.
  • There's a lot of code duplication. I don't see a good way out of this though (the obvious would be to have an "abstract" E.S. module but that would also be kinda overkill).

I hope this is useful feedback. A lot of code to review - and a very big contribution, so thanks!

@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 1, 2015

  • I think the Travis error is because I made an assumption: I couldn't find a way to get the ES node to tell what HTTP port it actually bound to (and I didn't want to force it to something that may conflict), so I just assumed 9201, which is normally the first port, and planned to look into it later if it gave complications on Travis. I guess that time has come! :)
  • I agree on the ES dependency. I'm not sure the code will be prettier using i.e. GSON, but shedding a huge dependency from a module (or at least making it test-only, we probably wont get rid of the embedded ES node) is always nice. Luckily, our use of the search functionality is rather limited, so I think it should be manageable.
  • I think I might a least be able to make a few things less duplicate-y, like wrapping the client's IOException inside MetaModelException. The request objects are generic and contains the type returned in Action<T extends JestResult>, so a simple wrapper should suffice.

Had to lock the HTTP port. I chose 9292 as that is pretty high in normal ES range, so improbable to be used already.
@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 2, 2015

Okay, I think I fixed the Travis error (we'll see soon), and I made the wrapper for the Exception.

There's still the ES dependency issue remaining, though

@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 2, 2015

D'oh. License.

@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 2, 2015

Fixed. Also found another silly mistake: I had used org.metamodel instead of org.apache.metamodel as the package name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

'logger' is never used

@kaspersorensen
Copy link
Copy Markdown
Contributor

Looks like very solid work. One general thought I have is to not prefix the classes with "Jest" though, but rather with "Rest". That way we don't tie it too much with the underlying library used, but rather with the protocol towards ElasticSearch. It will probably be more recognizable to ES users and it will also allow us to switch library and so on without changing class names.

@kaspersorensen
Copy link
Copy Markdown
Contributor

Please be aware that I raised another topic regarding this PR (and #68) in the mailing list under subject "[DISCUSS] ElasticSearch and MongoDB granularity"

@kaspersorensen kaspersorensen mentioned this pull request Nov 4, 2015
@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 4, 2015

I called it "ElasticSearchRestDataContext", and changed the package name to "org.apache.metamodel.elasticsearch.rest". I left the other class names as-is, since they are very much tied to Jest, and aren't part of the API.

@kaspersorensen
Copy link
Copy Markdown
Contributor

👍

@kaspersorensen
Copy link
Copy Markdown
Contributor

I would suggest to make the module restructure [1] already in this PR since it involves a bunch of code reuse. Or do you want to do that separately? What do you think?

[1] discussed in mail thread "[DISCUSS] ElasticSearch and MongoDB granularity" on dev@metamodel

@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 7, 2015

Merging them now would be fine.

@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 9, 2015

Reading it again, that was pretty unclear. With "merging", I meant making a common module now.

This adds a new package with common methods for both the native- and
rest-based clients. I'm actually not super fond with the solution,
but without completely restructuring the code (encapsulating all
communication into objects with common interfaces), I think this is
close to the best we can do.
@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 23, 2015

Whoops, forgot to comment that I added a new commit (it would be wonderful if Github pinged subscribers on new commits):
This adds a new package with common methods for both the native- and
rest-based clients. I'm actually not super fond with the solution,
but without completely restructuring the code (encapsulating all
communication into objects with common interfaces and introducing DTOs
for all requests and results), I think this is close to the best we can do.

The Travis failure seems unrelated, it's Excel going crazy in Java 8 for some reason.

@kaspersorensen
Copy link
Copy Markdown
Contributor

LGTM!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unused import

@kaspersorensen
Copy link
Copy Markdown
Contributor

Strange thing, in native/src/test/java the package is represented as one big folder instead of a tree of folders:

image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Map is raw type

@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 27, 2015

Yeah, I'm not exactly sure what happened with that folder. Fixed!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unused imports now ;)

@kaspersorensen
Copy link
Copy Markdown
Contributor

OK I think it's time to merge this ... Would you like to do the honors? I think maybe a squash first would be nice so that we have less commits on the master :-)

@asfgit asfgit closed this in 137caf0 Nov 27, 2015
@balubollam
Copy link
Copy Markdown

HI Everyone

I am implementing Jest client for Amazon client client connection.But iam getting this following error:
Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/http/ssl/SSLContexts
at org.apache.http.conn.ssl.SSLConnectionSocketFactory.getSocketFactory(SSLConnectionSocketFactory.java:172)
at io.searchbox.client.config.HttpClientConfig$Builder.build(HttpClientConfig.java:247)

Caused by: java.lang.ClassNotFoundException: org.apache.http.ssl.SSLContexts

But when i R&D on this i had come to know that
*I need to add patch to the httpcontext-osji using dependencies i.e adding pom.xml
Can Any one Please help me out how to add this patch or How to solve this error?

@LosD
Copy link
Copy Markdown
Contributor Author

LosD commented Nov 30, 2015

Hi @balubollam

When does this happen? During initialization of the MetaModel context, or when querying it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants