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.

Sign up for GitHub

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

Jump to bottom

Change conversion of hit in native client #40

Closed
konradkonrad opened this issue Sep 24, 2013 · 8 comments
Closed

Change conversion of hit in native client #40

konradkonrad opened this issue Sep 24, 2013 · 8 comments
Labels
compatibility request usability

Comments

@konradkonrad
Copy link

Currently the format of hits differs between rest and native in regard to the "fields"-key: native uses ":_fields" and rest uses ":fields".

I propose both to use ":fields".

To reproduce that issue:

# I assume, you have an elasticsearch cluster named "mytestcluster" 
# available at 127.0.0.1 on port 9200 (rest-http) and 9300 (native-transport) 
# respectively.

# Steps to reproduce
git clone http://github.com/clojurewerkz/elastisch
cd elastisch
lein repl

(require '[clojurewerkz.elastisch.rest :as r]
         '[clojurewerkz.elastisch.rest.document :as rd]
         '[clojurewerkz.elastisch.rest.index :as ri]
         '[clojurewerkz.elastisch.native :as n]
         '[clojurewerkz.elastisch.native.document :as nd])

(r/connect! "http://127.0.0.1:9200")

(let [mapping-types {:person :properties {:username {:type "string" :store "yes"}}}]
    (ri/create "mytest" :mappings mapping-types))

(rd/create "mytest" "person" {:username "tester"})

(n/connect! [["127.0.0.1" 9300]]
            {"cluster.name" "mytestcluster"})


(def resttest (keys
                (first
                  (:hits
                    (:hits
                      (rd/search-all-indexes-and-types
                        :fields ["username"]
                        :query {:match_all {}}))))))

(def nativetest (keys
                (first
                  (:hits
                    (:hits
                      (nd/search-all-indexes-and-types
                        :fields ["username"]
                        :query {:match_all {}}))))))

(println resttest nativetest)

;; (:_index :_type :_id :_score :fields) (:_version :_score :_id :_type :_index :_fields)
;; nil
;;
;; Should be:
;; (:_index :_type :_id :_score :fields) (:_version :_score :_id :_type :_index :fields)
;;

Change https://github.com/clojurewerkz/elastisch/blob/master/src/clojurewerkz/elastisch/native/conversion.clj#L535

- {:_fields (convert-fields-result (.getFields sh))})]
+ {:fields (convert-fields-result (.getFields sh))})]

PS: I was about to write a complete test-case and pull-request accordingly, but since I couldn't get the tests to run, I hope this is sufficient :)
@michaelklishin
Copy link
Member

I propose native client use both. Feel free to submit a pull request.

@michaelklishin
Copy link
Member

Sorry, "I could not get the tests to run" is not an acceptable argument. See .travis.yml to learn how it's done.

@schmir
Copy link

schmir commented Sep 25, 2013

michael thanks for the pointer to .travis.yml. I suggest you read http://blog.clojurewerkz.org/blog/2013/04/20/how-to-make-your-open-source-project-really-awesome/ :)

The documention on how to run the tests is missing. We just like to get some trivial stuff fixed and the missing documentation makes it harder than it should be.

Is the following output from lein test expected?

Ran 210 tests containing 1338 assertions.
8 failures, 0 errors.
Tests failed.

@michaelklishin
Copy link
Member

@schmir it would be more helpful if you contributed said documentation instead of trolling.

No, failures are not expected. CI passes and so do my local runs.

@michaelklishin
Copy link
Member

I probably should write a post on how to report issues in a way that helps maintainers. You did not provide any stack traces or other information about which tests fail.

ElasticSearch's native client has this annoying requirement that the client release should exactly match the ES release you run it against. Otherwise you may (and very likely, will) run into binary protocol compatibility issues.

Are you running the same release as Elastisch uses?

michaelklishin pushed a commit that referenced this issue Sep 25, 2013
Document how to run the test suite
d078e22 
References #40.
@konradkonrad
Copy link
Author

Hi michael!
Thanks a lot for implementing this. Seems I'm several minutes to late to send my pull request…
(My test issues were due to a non unique clustername, so thanks also for pointing this out in the documentation!)

@schmir
Copy link

schmir commented Sep 25, 2013

Thanks for the documentation.

lein test passes when I run it against an elasticsearch 0.90.3
(single-node) cluster.

If I run it against 0.90.5 I get some errors. I don't believe the
failures happen because of an incompatibility of the binary protocol
used since I get similar results when upgrading the dependency in
project.clj to 0.90.5.

Retrieving org/elasticsearch/elasticsearch/0.90.5/elasticsearch-0.90.5.pom from central
Retrieving org/elasticsearch/elasticsearch/0.90.5/elasticsearch-0.90.5.jar from central
Reflection warning, clj_http/core.clj:147:11 - reference to field close can't be resolved.
Using Clojure version {:major 1, :minor 5, :incremental 1, :qualifier nil}
Reflection warning, clojurewerkz/elastisch/native/conversion.clj:127:9 - call to put can't be resolved.
Reflection warning, clojurewerkz/elastisch/native/conversion.clj:127:9 - call to put can't be resolved.
Reflection warning, clojurewerkz/elastisch/native/conversion.clj:243:54 - reference to field getValue can't be resolved.
Reflection warning, clojurewerkz/elastisch/native/conversion.clj:463:7 - call to stats can't be resolved.
Reflection warning, clojurewerkz/elastisch/native/conversion.clj:796:7 - call to refresh can't be resolved.
Reflection warning, clojurewerkz/elastisch/native/conversion.clj:808:7 - call to refresh can't be resolved.
Sep 25, 2013 1:23:28 PM org.elasticsearch.plugins
INFO: [Eel] loaded [], sites []
lein test clojurewerkz.elastisch.fixtures
lein test clojurewerkz.elastisch.internal.rest-test
lein test clojurewerkz.elastisch.internal.utils-test
lein test clojurewerkz.elastisch.native-api.count-test
lein test clojurewerkz.elastisch.native-api.delete-test
lein test clojurewerkz.elastisch.native-api.facets-test
lein test clojurewerkz.elastisch.native-api.get-test
lein test clojurewerkz.elastisch.native-api.indexing-test
lein test clojurewerkz.elastisch.native-api.indices-test
lein test :only clojurewerkz.elastisch.native-api.indices-test/test-flush-index-with-refresh
ERROR in (test-flush-index-with-refresh) (Reflector.java:53)
Uncaught exception, not in assertion.
expected: nil
  actual: java.lang.IllegalArgumentException: No matching method found: refresh for class org.elasticsearch.action.admin.indices.flush.FlushRequest
 at clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)
    clojure.lang.Reflector.invokeInstanceMethod (Reflector.java:28)
    clojurewerkz.elastisch.native.conversion$__GT_flush_index_request.invoke (conversion.clj:808)
    clojurewerkz.elastisch.native.index$flush.doInvoke (index.clj:122)
    clojure.lang.RestFn.invoke (RestFn.java:439)
    clojurewerkz.elastisch.native_api.indices_test/fn (indices_test.clj:71)
    clojure.test$test_var$fn__7145.invoke (test.clj:701)
    clojure.test$test_var.invoke (test.clj:701)
    clojure.test$test_all_vars$fn__7149$fn__7156.invoke (test.clj:717)
    clojurewerkz.elastisch.fixtures$reset_indexes.invoke (fixtures.clj:16)
    clojure.test$compose_fixtures$fn__7139$fn__7140.invoke (test.clj:678)
    clojure.test$default_fixture.invoke (test.clj:671)
    clojure.test$compose_fixtures$fn__7139.invoke (test.clj:678)
    clojure.test$test_all_vars$fn__7149.invoke (test.clj:717)
    clojure.test$default_fixture.invoke (test.clj:671)
    clojure.test$test_all_vars.invoke (test.clj:713)
    clojure.test$test_ns.invoke (test.clj:736)
    clojure.core$map$fn__4207.invoke (core.clj:2487)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:60)
    clojure.lang.Cons.next (Cons.java:39)
    clojure.lang.RT.next (RT.java:598)
    clojure.core$next.invoke (core.clj:64)
    clojure.core$reduce1.invoke (core.clj:896)
    clojure.core$reduce1.invoke (core.clj:887)
    clojure.core$merge_with.doInvoke (core.clj:2702)
    clojure.lang.RestFn.applyTo (RestFn.java:139)
    clojure.core$apply.invoke (core.clj:619)
    clojure.test$run_tests.doInvoke (test.clj:751)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.core$apply.invoke (core.clj:617)
    user$eval87$fn__142$fn__173.invoke (form-init1162413116088527593.clj:1)
    user$eval87$fn__142$fn__143.invoke (form-init1162413116088527593.clj:1)
    user$eval87$fn__142.invoke (form-init1162413116088527593.clj:1)
    user$eval87.invoke (form-init1162413116088527593.clj:1)
    clojure.lang.Compiler.eval (Compiler.java:6619)
    clojure.lang.Compiler.eval (Compiler.java:6609)
    clojure.lang.Compiler.load (Compiler.java:7064)
    clojure.lang.Compiler.loadFile (Compiler.java:7020)
    clojure.main$load_script.invoke (main.clj:294)
    clojure.main$init_opt.invoke (main.clj:299)
    clojure.main$initialize.invoke (main.clj:327)
    clojure.main$null_opt.invoke (main.clj:362)
    clojure.main$main.doInvoke (main.clj:440)
    clojure.lang.RestFn.invoke (RestFn.java:421)
    clojure.lang.Var.invoke (Var.java:419)
    clojure.lang.AFn.applyToHelper (AFn.java:163)
    clojure.lang.Var.applyTo (Var.java:532)
    clojure.main.main (main.java:37)
lein test clojurewerkz.elastisch.native-api.mappings-test
lein test clojurewerkz.elastisch.native-api.more-like-this-test
lein test clojurewerkz.elastisch.native-api.percolation-test
lein test clojurewerkz.elastisch.native-api.queries.bool-query-test
lein test clojurewerkz.elastisch.native-api.queries.custom-score-query-test
lein test clojurewerkz.elastisch.native-api.queries.field-query-test
lein test clojurewerkz.elastisch.native-api.queries.filtered-query-test
lein test clojurewerkz.elastisch.native-api.queries.flt-field-query-test
lein test clojurewerkz.elastisch.native-api.queries.flt-query-test
lein test clojurewerkz.elastisch.native-api.queries.fuzzy-query-test
lein test clojurewerkz.elastisch.native-api.queries.ids-query-test
lein test clojurewerkz.elastisch.native-api.queries.match-all-query-test
lein test clojurewerkz.elastisch.native-api.queries.mlt-field-query-test
lein test clojurewerkz.elastisch.native-api.queries.mlt-query-test
lein test clojurewerkz.elastisch.native-api.queries.prefix-query-test
lein test clojurewerkz.elastisch.native-api.queries.query-string-query-test
lein test clojurewerkz.elastisch.native-api.queries.range-query-test
lein test clojurewerkz.elastisch.native-api.queries.term-query-test
lein test :only clojurewerkz.elastisch.native-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-usernames
FAIL in (test-basic-term-query-over-non-analyzed-usernames) (term_query_test.clj:49)
expected: (= "4" (-> (doc/search "tweets" "tweet" :query (q/term :username "michaelklishin")) hits-from first :_id))
  actual: (not (= "4" "3"))
lein test :only clojurewerkz.elastisch.native-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-usernames
FAIL in (test-basic-term-query-over-non-analyzed-usernames) (term_query_test.clj:49)
expected: (is (= "4" (-> (doc/search "tweets" "tweet" :query (q/term :username "michaelklishin")) hits-from first :_id)))
  actual: false
lein test :only clojurewerkz.elastisch.native-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-embedded-fields
FAIL in (test-basic-term-query-over-non-analyzed-embedded-fields) (term_query_test.clj:59)
expected: (= "4" (-> (doc/search "tweets" "tweet" :query (q/term "location.state" "Moscow")) hits-from first :_id))
  actual: (not (= "4" "1"))
lein test :only clojurewerkz.elastisch.native-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-embedded-fields
FAIL in (test-basic-term-query-over-non-analyzed-embedded-fields) (term_query_test.clj:59)
expected: (is (= "4" (-> (doc/search "tweets" "tweet" :query (q/term "location.state" "Moscow")) hits-from first :_id)))
  actual: false
lein test clojurewerkz.elastisch.native-api.queries.text-query-test
lein test clojurewerkz.elastisch.native-api.queries.wildcard-query-test
lein test clojurewerkz.elastisch.native-api.search-scroll-test
lein test clojurewerkz.elastisch.native-api.search-test
lein test clojurewerkz.elastisch.native-api.update-test
lein test clojurewerkz.elastisch.query-test
lein test clojurewerkz.elastisch.rest-api.bulk-test
lein test clojurewerkz.elastisch.rest-api.count-test
lein test clojurewerkz.elastisch.rest-api.delete-test
lein test clojurewerkz.elastisch.rest-api.facets-test
lein test clojurewerkz.elastisch.rest-api.get-test
lein test clojurewerkz.elastisch.rest-api.indexing-test
lein test clojurewerkz.elastisch.rest-api.indices-test
lein test clojurewerkz.elastisch.rest-api.mappings-test
lein test clojurewerkz.elastisch.rest-api.more-like-this-test
lein test clojurewerkz.elastisch.rest-api.multi-test
lein test clojurewerkz.elastisch.rest-api.percolation-test
lein test clojurewerkz.elastisch.rest-api.queries.bool-query-test
lein test clojurewerkz.elastisch.rest-api.queries.custom-score-query-test
lein test clojurewerkz.elastisch.rest-api.queries.field-query-test
lein test clojurewerkz.elastisch.rest-api.queries.filtered-query-test
lein test clojurewerkz.elastisch.rest-api.queries.flt-field-query-test
lein test clojurewerkz.elastisch.rest-api.queries.flt-query-test
lein test clojurewerkz.elastisch.rest-api.queries.fuzzy-query-test
lein test clojurewerkz.elastisch.rest-api.queries.ids-query-test
lein test clojurewerkz.elastisch.rest-api.queries.match-all-query-test
lein test clojurewerkz.elastisch.rest-api.queries.mlt-field-query-test
lein test clojurewerkz.elastisch.rest-api.queries.mlt-query-test
lein test clojurewerkz.elastisch.rest-api.queries.prefix-query-test
lein test clojurewerkz.elastisch.rest-api.queries.query-string-query-test
lein test clojurewerkz.elastisch.rest-api.queries.range-query-test
lein test clojurewerkz.elastisch.rest-api.queries.term-query-test
lein test :only clojurewerkz.elastisch.rest-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-usernames
FAIL in (test-basic-term-query-over-non-analyzed-usernames) (term_query_test.clj:47)
expected: (= "4" (-> (doc/search "tweets" "tweet" :query (q/term :username "michaelklishin")) hits-from first :_id))
  actual: (not (= "4" "3"))
lein test :only clojurewerkz.elastisch.rest-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-usernames
FAIL in (test-basic-term-query-over-non-analyzed-usernames) (term_query_test.clj:47)
expected: (is (= "4" (-> (doc/search "tweets" "tweet" :query (q/term :username "michaelklishin")) hits-from first :_id)))
  actual: false
lein test :only clojurewerkz.elastisch.rest-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-embedded-fields
FAIL in (test-basic-term-query-over-non-analyzed-embedded-fields) (term_query_test.clj:57)
expected: (= "4" (-> (doc/search "tweets" "tweet" :query (q/term "location.state" "Moscow")) hits-from first :_id))
  actual: (not (= "4" "1"))
lein test :only clojurewerkz.elastisch.rest-api.queries.term-query-test/test-basic-term-query-over-non-analyzed-embedded-fields
FAIL in (test-basic-term-query-over-non-analyzed-embedded-fields) (term_query_test.clj:57)
expected: (is (= "4" (-> (doc/search "tweets" "tweet" :query (q/term "location.state" "Moscow")) hits-from first :_id)))
  actual: false
lein test clojurewerkz.elastisch.rest-api.queries.text-query-test
lein test clojurewerkz.elastisch.rest-api.queries.wildcard-query-test
lein test clojurewerkz.elastisch.rest-api.search-scroll-test
lein test clojurewerkz.elastisch.rest-api.search-test
lein test clojurewerkz.elastisch.rest-api.update-test
lein test clojurewerkz.elastisch.test.helpers
Ran 210 tests containing 1338 assertions.
8 failures, 1 errors.
Tests failed.

@michaelklishin
Copy link
Member

I've released 1.3.0-beta3 with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compatibility request usability
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@michaelklishin @schmir @konradkonrad