Improve scrolls: fix scroll ttl bug and implement scroll-clear

[axel-angel]

In the current version, the :scroll argument isn't preserved after the 2nd call, only the first one will have the correct ttl. This bug is fixed in the 1st commit.

Meanwhile, because big ttl can have a big performance on indexing, the deletion of scroll is implemented in this PR as well to reduce this problem.

[michaelklishin]

I think clear-scroll reads a bit better.

[michaelklishin]

@axel-angel thank you. Can you please add a test that verifies that the bug is gone?

[axel-angel]

@michaelklishin I have no idea what's happening in the tests. I couldn't reproduce the test environnement on my machine, it seems there are two instances of ES and I'm confused.

[michaelklishin]

@axel-angel sorry, is your question basically "how do I run the tests"?

[Josh-Tilles]

@michaelklishin it might be that he's having trouble reproducing the Travis CI failures locally.

[axel-angel]

Nevermind, I reproduced with lein all test :ci, I'm taking a look.

[axel-angel]

After a bit of thinking, I cannot see how to test this scroll TTL, of course without a sleep. Maybe with a proxy function in the test and checking arguments, any idea?

[Josh-Tilles]

FYI you didn’t have to move the default value down below; it would've worked if you changed :or {scroll "1m"} to :or {scroll-ttl "1m"}.

[axel-angel]

Thanks! I didn't know it was the destination key and not the source.

[Josh-Tilles]

To be honest, neither did I until I did a bit of REPL experimentation as a result of looking over this PR ;)

[michaelklishin]

Using a sleep is a sad fact of life for data service client tests since there are no notifications for many events internal to the server. So go for it.

[axel-angel]

Would it be sufficient to test whether the scroll function receive the ttl each time? I can test it using with-redefs and checking arguments.

[michaelklishin]

@axel-angel I'd really prefer to have a "scrolled" result set at the end, much like if it was a non-scrolled query. It should easier in Clojure than in many other languages, for example, simply append things to an atom containing a list ;)

[axel-angel]

@michaelklishin The code I modified shouldn't change the result nor its type at all or I missed something? Or are you asking to take this PR as an opportunity to change something else? I didn't understand what you actually meant, can you elaborate? Thanks.

[michaelklishin]

@axel-angel I'm asking for a test that reproduces the issue without your change and passes with it.

[michaelklishin]

OK, I guess for the sake of releasing a preview this w/e, we should merge this and worry about adding a test for this specific scrolling case later.

[axel-angel]

@michaelklishin I tried to write this simple test but it didn't failed:

  (deftest ^{:rest true :scroll true} test-scroll-seq-scan-expire
    (let [index-name   "articles"
          mapping-type "article"
          res-seq      (doc/scroll-seq conn
                                       (doc/search conn index-name mapping-type
                                                   {:query (q/match-all)
                                                    :search_type "scan"
                                                    :scroll "1s"
                                                    :size 2})
                                       {:search_type "scan", :scroll "1s"})]
      (Thread/sleep 3000)
      (is (not (realized? res-seq)))
      (is (= 4 (count (distinct res-seq))))
      (is (realized? res-seq))))

The scroll should expire right away however it doesn't crash. Any idea why?

[michaelklishin]

@axel-angel are there enough results available for the 2nd "scroll" to happen?

[Josh-Tilles]

@axel-angel Also, keep in mind that the :size option might be applied per shard, not per request. (At least, that was the case in 1.7 and in 2.0, but Elastic removed that part of the documentation in subsequent versions, so I'm not sure what the behavior is now.)