Introducing "took" time (in ms) for `_msearch` #23767

olcbean · 2017-03-27T17:27:36Z

As per #23131 (comment) it is not clear if introducing took time for multi search is a desired behavior or if a further discussion is needed.

But as there are no comments against it so far I am submitting this PR.

elasticmachine · 2017-03-27T17:27:38Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

olcbean · 2017-03-27T17:30:47Z

Hm...
I signed the CLA, but still the check is failing...

jasontedor

I left some nits, but they're important as reverting these changes will make the PR smaller.

jasontedor · 2017-03-28T01:27:05Z

core/src/test/java/org/elasticsearch/action/search/TransportMultiSearchActionTests.java

+        TransportService transportService = new TransportService(Settings.EMPTY, null, null,
+                TransportService.NOOP_TRANSPORT_INTERCEPTOR,
+                boundAddress -> DiscoveryNode.createLocal(settings, boundAddress.publishAddress(),
+                        UUIDs.randomBase64UUID()), null) {


Please revert all line-length changes that are changing lines that otherwise were not impacted by the changes in this PR.

jasontedor · 2017-03-28T01:27:44Z

core/src/test/java/org/elasticsearch/action/search/TransportMultiSearchActionTests.java

+
+        TransportMultiSearchAction action = new TransportMultiSearchAction(threadPool,
+                actionFilters, transportService, clusterService, searchAction, resolver, 10,
+                System::nanoTime);


As far as I can tell, this is the only change in this file that was necessitated by the changes in this PR.

jasontedor · 2017-03-28T01:28:27Z

core/src/test/java/org/elasticsearch/action/search/MultiSearchActionTookTests.java

+        clusterService.close();
+    }
+
+    private enum Clock {


Please remove, a simple boolean like in the other took tests is fine.

It was added to readability but I will replace it with a boolean.

olcbean · 2017-03-28T09:41:38Z

@jasontedor I hope I addressed all of your remarks.
Thank you for your review :)

jasontedor

Thanks so much @olcbean, I left a first-round review.

jasontedor · 2017-04-04T02:09:39Z

core/src/main/java/org/elasticsearch/action/search/MultiSearchResponse.java


    MultiSearchResponse() {
    }

-    public MultiSearchResponse(Item[] items) {
+    public MultiSearchResponse(long tookInMillis, Item[] items) {


Let's flip the order of these parameters (in general, I think that new parameters should be added at the end absent a good reason otherwise, but putting it at the end also aligns it more closely with the BulkResponse; it wouldn't surprise me we are not consistent about this through the codebase, but I'm choosing the BulkResponse here to be consistent with).

jasontedor · 2017-04-04T02:09:55Z

core/src/main/java/org/elasticsearch/action/search/MultiSearchResponse.java


    MultiSearchResponse() {
    }

-    public MultiSearchResponse(Item[] items) {
+    public MultiSearchResponse(long tookInMillis, Item[] items) {
+        this.tookInMillis = tookInMillis;


Let's move this parameter down after the order of the parameter is changed.

jasontedor · 2017-04-04T02:11:25Z

core/src/main/java/org/elasticsearch/action/search/MultiSearchResponse.java

@@ -111,11 +112,14 @@ public Exception getFailure() {
    }

    private Item[] items;
+


Can you remove this unnecessary space?

jasontedor · 2017-04-04T02:12:37Z

core/src/main/java/org/elasticsearch/action/search/MultiSearchResponse.java

+    /**
+     * How long the msearch took.
+     */
+    public TimeValue getTook() {


It looks like this is only used in tests, and only to get the milliseconds, so I think that we can remove it and just use getTookInMillis in those places.

The method exists in both SearchResponse and BulkResponse.
I would prefer to keep the consistency unless there is a good reason no to.

There is a good reason, the method is unneeded.

So shall I remove also the corresponding methods for Bulk and Search?

Not as part of this change.

jasontedor · 2017-04-04T02:13:33Z

core/src/main/java/org/elasticsearch/action/search/MultiSearchResponse.java

@@ -138,6 +156,7 @@ public void readFrom(StreamInput in) throws IOException {
        for (int i = 0; i < items.length; i++) {
            items[i] = Item.readItem(in);
        }
+        tookInMillis = in.readVLong();


There are backwards compatibility concerns here. Consider a mixed-version cluster that contains nodes that do not know about took. You need to guard this read in a version check on the stream. For now, let's try for on or after 5.4.0. We can adjust this later if needed.

jasontedor · 2017-04-04T02:26:25Z

core/src/test/java/org/elasticsearch/action/search/MultiSearchActionTookTests.java

+
+    @After
+    public void tearDown() throws Exception {
+        super.tearDown();


The super teardown should be done after the specific teardown.

Agreed. However I was closely following the TransportBulkActionTookTests. Maybe you should change it there too?

jasontedor · 2017-04-04T02:28:02Z

core/src/test/java/org/elasticsearch/action/search/MultiSearchActionTookTests.java

+        clusterService.close();
+    }
+
+    //Unit conversion using a controller clock


Can you add a space between the // and Unit, add the word test and lowercase Unit?

jasontedor · 2017-04-04T02:28:33Z

core/src/test/java/org/elasticsearch/action/search/MultiSearchActionTookTests.java

+        runTestTook(true);
+    }
+
+    //Using {@link System#nanoTime()}


Let's remove the use of Javadoc syntax here since this is not a Javadoc, add a space between // and Using, add the word test, and lowercase Using.

jasontedor · 2017-04-04T02:28:52Z

core/src/test/java/org/elasticsearch/action/search/MultiSearchActionTookTests.java

+
+            @Override
+            public void onFailure(Exception e) {
+                logger.warn(e);


This should probably be a fail.

jasontedor · 2017-04-04T02:29:32Z

core/src/test/java/org/elasticsearch/action/search/MultiSearchIT.java

+/**
+ * MultiSearch IT
+ */
+public class MultiSearchIT extends ESSingleNodeTestCase {


I do not think that we need an integration test for this. I appreciate that you wrote it, but I think unit test suffices.

dakrone · 2017-08-15T19:51:44Z

@olcbean thanks for opening this! It looks like it needs to have master merged in to fix some of the version constants, also, is it ready for another review by @jasontedor?

olcbean · 2017-08-16T09:11:48Z

@dakrone thanks for following up on this :)

I am currently traveling, will rebase at the start of next week.

removing MultiSearchIT adding a note to the migration docs

olcbean · 2017-08-23T21:22:22Z

@jasontedor I just rebased on top of the current master and set the bwc version to 7.0 ( perhaps should be changed to 6.0 ? ). I also added a short note to the migration docs - feel free to leave it off if the change is not sufficiently notable.

Could you please trigger the CI and let me know if any further changes are needed.

CC-ing @dakrone as he also expressed interest in this PR.

dakrone · 2017-08-23T21:29:01Z

@elasticmachine test this please

olcbean · 2017-09-23T09:20:18Z

It has been about a month with no communication on this PR. Could you please let me know what the next steps are? @jasontedor @dakrone @rjernst

* master: (356 commits) Do not set SO_LINGER on server channels (elastic#26997) Fix inconsistencies in the rest api specs for *_script (elastic#26971) fix inconsistencies in the rest api specs for cat.snapshots (elastic#26996) Add docs on full_id parameter in cat nodes API [TEST] Add test that replicates versioned updates with random flushes Use internal searcher for all indexing related operations in the engine Reformat paragraph in template docs to 80 columns Clarify settings and template on create index Fix reference to TcpTransport in documentation Allow Uid#decodeId to decode from a byte array slice (elastic#26987) Fix a typo in the similarity docs (elastic#26970) Use separate searchers for "search visibility" vs "move indexing buffer to disk (elastic#26972) Create weights lazily in filter and filters aggregation (elastic#26983) Use a dedicated ThreadGroup in rest sniffer (elastic#26897) Fire global checkpoint sync under system context Update by Query is modified to accept short `script` parameter. (elastic#26841) Cat shards bytes (elastic#26952) Add support for parsing inline script (elastic#23824) (elastic#26846) Change default value to true for transpositions parameter of fuzzy query (elastic#26901) Adding unreleased 5.6.4 version number to Version.java ...

jasontedor

I left a few comments.

jasontedor · 2017-10-16T08:15:31Z

core/src/main/java/org/elasticsearch/action/search/TransportMultiSearchAction.java


 public class TransportMultiSearchAction extends HandledTransportAction<MultiSearchRequest, MultiSearchResponse> {

    private final int availableProcessors;
    private final ClusterService clusterService;
    private final TransportAction<SearchRequest, SearchResponse> searchAction;
+    private final LongSupplier relativeTimeProvider;
+    private SetOnce<Long> startTimeInNanos;


I don't think SetOnce<Long> here is desirable, that's two extra allocations on every multi-search request.

The immutability is a positive though, we can obtain this by pushing the start time all the way down via the execute method (i.e., add it as a parameter and chain it through all the way down to finish).

@jasontedor good point! Thanks.

olcbean · 2017-10-16T12:04:01Z

@jasontedor thank you for picking up this PR. Can you have another look?

jasontedor

I left a few more comments.

jasontedor · 2017-10-16T12:08:48Z

core/src/main/java/org/elasticsearch/action/search/TransportMultiSearchAction.java

@@ -123,7 +121,8 @@ void executeSearch(
            final Queue<SearchRequestSlot> requests,
            final AtomicArray<MultiSearchResponse.Item> responses,
            final AtomicInteger responseCounter,
-            final ActionListener<MultiSearchResponse> listener) {
+            final ActionListener<MultiSearchResponse> listener,
+            long startTimeInNanos) {


Please make this parameter final, and name it relativeStartTime (to make it clear that it's start time, but it's a relative and not absolute start time).

jasontedor · 2017-10-16T12:09:11Z

core/src/main/java/org/elasticsearch/action/search/TransportMultiSearchAction.java

@@ -72,7 +70,7 @@ public TransportMultiSearchAction(Settings settings, ThreadPool threadPool, Tran

    @Override
    protected void doExecute(MultiSearchRequest request, ActionListener<MultiSearchResponse> listener) {
-        startTimeInNanos = new SetOnce<>(relativeTime());
+        long startTimeInNanos = relativeTime();


This can be final, and change it's name relativeStartTime.

jasontedor · 2017-10-16T12:10:10Z

core/src/test/java/org/elasticsearch/action/search/MultiSearchActionTookTests.java

@@ -164,9 +165,9 @@ protected void doExecute(SearchRequest request, ActionListener<SearchResponse> l
                    availableProcessors, expected::get) {
                @Override
                void executeSearch(final Queue<SearchRequestSlot> requests, final AtomicArray<MultiSearchResponse.Item> responses,
-                        final AtomicInteger responseCounter, final ActionListener<MultiSearchResponse> listener) {
+                        final AtomicInteger responseCounter, final ActionListener<MultiSearchResponse> listener, long startTimeInNanos) {


Same suggestion about the addition parameter.

jasontedor · 2017-10-16T12:11:04Z

core/src/main/java/org/elasticsearch/action/search/TransportMultiSearchAction.java

            }
        });
    }

+    private long relativeTime() {
+        return relativeTimeProvider.getAsLong();


I don't think this method is needed.

olcbean · 2017-10-16T15:46:01Z

@jasontedor can you have another look?

jasontedor · 2017-10-16T21:24:27Z

test this please

olcbean · 2017-10-25T19:04:20Z

@jasontedor so how does it look?

jasontedor

There is one more change I would like, then this can be pulled in.

jasontedor · 2017-10-30T12:37:34Z

docs/reference/migration/migrate_7_0/java.asciidoc

@@ -0,0 +1,9 @@
+[[breaking_70_java_changes]]


We typically do not add changes here for feature adds, preferring to reserve it for changes users need to know to help them migrate; I see this as a feature add. Could you please remove this?

olcbean · 2017-10-30T12:56:14Z

@jasontedor just pushed the latest changes. Take another look?

jasontedor · 2017-10-30T13:19:34Z

test this please

* master: (63 commits) [Docs] Fix note in bucket_selector [Docs] Fix indentation of examples (elastic#27168) [Docs] Clarify `span_not` query behavior for non-overlapping matches (elastic#27150) [Docs] Remove first person "I" from getting started (elastic#27155) [Docs] Correct link target for datatype murmur3 (elastic#27143) Fix division by zero in phrase suggester that causes assertion to fail Enable Docstats with totalSizeInBytes for 6.1.0 Adds average document size to DocsStats (elastic#27117) Upgrade Painless from ANTLR 4.5.1-1 to ANTLR 4.5.3. (elastic#27153) Exists template needs a template name (elastic#25988) [Tests] Fix occasional test failure due to two random values being the same Fix beidermorse phonetic token filter for unspecified `languageset` (elastic#27112) Fix max score tracking with field collapsing (elastic#27122) [Doc] Add Ingest CSV Processor Plugin to plugin as a community plugin (elastic#27105) Removed the beta tag from cross-cluster search fixed typo in ConstructingObjectParse (elastic#27129) Allow for the Painless Definition to have multiple instances (elastic#27096) Apply missing request options to the expand phase (elastic#27118) Only pull SegmentReader once in getSegmentInfo (elastic#27121) Fix BWC for discovery stats ...

jasontedor · 2017-10-30T16:11:04Z

test this please

jasontedor

LGTM.

jasontedor · 2017-11-02T01:39:22Z

Thanks @olcbean.

elasticmachine · 2017-11-06T10:40:36Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Implements: elastic/elasticsearch#23767

jasontedor requested changes Mar 28, 2017

View reviewed changes

jasontedor requested changes Apr 4, 2017

View reviewed changes

olcbean added 5 commits August 23, 2017 20:39

Introducing "took" time (in ms) for _msearch elastic#23131

b0b06e5

Enforce line width 100

8317497

Modifications to address the reviewer's remarks

b7c41ce

Modifications to address the reviewer's remarks

83f77ea

rebase and set the bwc to 7.0

c6ad0cc

removing MultiSearchIT adding a note to the migration docs

olcbean force-pushed the msearch_took branch from 6ce136e to c6ad0cc Compare August 23, 2017 21:14

jasontedor requested changes Oct 16, 2017

View reviewed changes

pushing the start time all the way down to finish

ccae5b4

jasontedor requested changes Oct 16, 2017

View reviewed changes

integrating reviewers comments

516937a

jasontedor requested changes Oct 30, 2017

View reviewed changes

removing java.asciidoc

7ced928

jasontedor approved these changes Nov 2, 2017

View reviewed changes

jasontedor added :Search/Search Search-related issues that do not fall into other categories v7.0.0 >enhancement labels Nov 2, 2017

jasontedor merged commit b989646 into elastic:master Nov 2, 2017

jasontedor mentioned this pull request Nov 2, 2017

What is the "took" value for multi-search query #23131

Closed

olcbean deleted the msearch_took branch November 2, 2017 08:53

clintongormley changed the title ~~Introducing "took" time (in ms) for _msearch #23131~~ Introducing "took" time (in ms) for _msearch Nov 6, 2017

javanna mentioned this pull request May 28, 2018

Add MultiSearchTemplate support to High Level Rest client #30836

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

codebrain added a commit to elastic/elasticsearch-net that referenced this pull request Jun 20, 2019

Adding took time to _msearch

e73bdc4

Implements: elastic/elasticsearch#23767

codebrain mentioned this pull request Jun 20, 2019

Adding took time to _msearch elastic/elasticsearch-net#3841

Merged

Mpdreamz pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 20, 2019

Adding took time to _msearch (#3841)

32593bd

Implements: elastic/elasticsearch#23767

		@@ -111,11 +112,14 @@ public Exception getFailure() {
		}

		private Item[] items;

Introducing "took" time (in ms) for _msearch #23767

Introducing "took" time (in ms) for _msearch #23767

Conversation

olcbean commented Mar 27, 2017

elasticmachine commented Mar 27, 2017

olcbean commented Mar 27, 2017

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Mar 28, 2017

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakrone commented Aug 15, 2017

olcbean commented Aug 16, 2017

olcbean commented Aug 23, 2017

dakrone commented Aug 23, 2017

olcbean commented Sep 23, 2017

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Oct 16, 2017

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Oct 16, 2017

jasontedor commented Oct 16, 2017

olcbean commented Oct 25, 2017

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

olcbean commented Oct 30, 2017

jasontedor commented Oct 30, 2017

jasontedor commented Oct 30, 2017

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor commented Nov 2, 2017

elasticmachine commented Nov 6, 2017

Introducing "took" time (in ms) for `_msearch` #23767

Introducing "took" time (in ms) for `_msearch` #23767

olcbean Apr 4, 2017 •

edited

Loading

jasontedor Oct 30, 2017 •

edited

Loading