Date
21 - 28 of 28
[DISCUSS] Elasticsearch Http using Jest
|
sjudeng <sju...@...>
Keith, I added a PR comment regarding testing before seeing this. Definitely would welcome your efforts on any of the items you mentioned. Probably need some consensus here regarding dropping transport client (though I'm a +1). I'm a little concerned on the integration test stuff as I think there's value in having a full ES instance for testing. But on the flip side it would be pretty compelling if this would clean up the implementation and still support conclusively testing against any (supported) ES distribution version. |
|
|
|
Keith Lohnes <loh...@...>
>clean up the implementation and still support conclusively testing against any (supported) ES distribution version. That's my main driver for this. I see having that transport client as pretty unnecessary. It's not compiling right now since I'm cleaning things up and working on tests, but if you want to take a look at what I'm doing, I opened up [a PR](https://github.com/JanusGraph/janusgraph/pull/160) On Wed, Mar 8, 2017 at 1:17 PM sjudeng <sju...@...> wrote:
|
|
|
|
sjudeng <sju...@...>
Keith, Sounds good. Are you going to rebase those commits on mine or keep them separate for now? I want to avoid painful merge/conflict issues for either of us later down the line. Jason, Are we close to moving forward here? I propose we create an issue requesting an initial JanusGraph release based on a commit prior to breaking changes. This would mitigate the concern regarding ES 1.x compatibility. This might be a good idea anyway to address ongoing questions on a timeline for an initial JanusGraph release. If that's separated out from ongoing dependency update work then I think it could get put together relatively quickly on a separate release branch. In parallel to the initial compatibility release work, we could then proceed with (among other things) ES development. On this I propose:
|
|
|
|
Jason Plurad <plu...@...>
sjudeng, I think the ES plan sounds good. This thread has been open for a while, and we have gotten any other input from the community. It sounds fair to start up a [VOTE] thread. Re: creating the initial JanusGraph release prior to breaking changes, that's a separate thread all together. On Wednesday, March 8, 2017 at 2:58:46 PM UTC-5, sjudeng wrote:
|
|
|
|
sjudeng <sju...@...>
I added a comment to the existing release discussion regarding creating an initial compatibility release and started a vote thread. https://groups.google.com/d/msg/janusgraph-dev/8jkMnkKzmC0/nvfgaChmBQAJ https://groups.google.com/forum/#!topic/janusgraph-dev/FLWuQMcW8-4 |
|
|
|
sjudeng <sju...@...>
Keith, I looked over the latest commit on your Jest update branch. Do you think the update to support running tests through ESIntegTestCase will be worth the additional complexity of needing to separate out into multiple ES test modules? Personally my preference in this case would be to stay with the approach of testing through a full instance via the ES distribution artifact available since 2.x and added in #79. I think there's value in running tests against an actual (fully unmocked) ES instance, the implementation is cleaner, and it supports testing against any 2.x and 5.x distribution. We already have approval to drop support for 1.x (after the 0.1.0 release). Also since I think we have consensus on the path forward to update ES (merging #79 and then migrating to Jest), would you consider retargeting your Jest update commits onto the branch in #79? My hope is we'll be able to get #79 merged after the 0.1.0 release is out/started. I think your updates to use Jest will improve that implementation by removing all the custom REST request/response objects I added. Even better it'll also remove the ES dependency (right?), which will free us up for the Lucene upgrade. Thanks for your work on this and for making it visible via your WIP PR. However we proceed I think communicating early and often is helpful here. |
|
|
|
Keith Lohnes <loh...@...>
sjudeng, thanks for looking things over. I've moved away from using the ESIntegTestCase. I couldn't get it to work. The reason for separating out the was 1.x support, so I'll likely undo that change. I also think retargeting the changes on to 79 is a good idea as well. I've been keeping an eye on the Jest repo and it looks like things are starting to move a little bit on the 5.x support, which is why I started back up on coding on that PR a little. >Even better it'll also remove the ES dependency It won't. There's still a few things in the code that rely on the ES dependency. Here's the list. ``` import org.elasticsearch.Version; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.search.sort.FieldSortBuilder; import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.index.query.OrFilterBuilder; import org.elasticsearch.index.query.AndFilterBuilder; import org.elasticsearch.index.query.FilterBuilder; import org.elasticsearch.index.query.FilterBuilders; import org.elasticsearch.index.query.QueryBuilders; ``` These could probably be replaced, but I hadn't really intended to do that work right now. So it will get things closer to being able to remove that ES dependency, but it's not _quite_ there yet. On Fri, Mar 24, 2017 at 7:54 PM sjudeng <sju...@...> wrote:
|
|
|
|
sjudeng <sju...@...>
Sounds good. Hopefully the Jest 5.x jar will be backwards compatible with ES 2.x in the same way the ES REST client is. |
|
|