From 2d9c000ada4203c688177d43653bdf6177007fc9 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Thu, 11 Jan 2018 18:34:17 -0500 Subject: [PATCH 1/7] Do not keep 5.x commits once having 6.x commits (#28188) Currently we keep a 5.x index commit as a safe commit until we have a 6.x safe commit. During that time, if peer-recovery happens, a primary will send a 5.x commit in file-based sync and the recovery will even fail as the snapshotted commit does not have sequence number tags. This commit updates the combined deletion policy to delete legacy commits if there are 6.x commits. Relates #27606 Relates #28038 --- .../elasticsearch/index/engine/CombinedDeletionPolicy.java | 4 ++-- .../index/engine/CombinedDeletionPolicyTests.java | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/engine/CombinedDeletionPolicy.java b/server/src/main/java/org/elasticsearch/index/engine/CombinedDeletionPolicy.java index d603a40b5d3ff..ffb12f5b84381 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/CombinedDeletionPolicy.java +++ b/server/src/main/java/org/elasticsearch/index/engine/CombinedDeletionPolicy.java @@ -121,9 +121,9 @@ private static int indexOfKeptCommits(List commits, long if (expectedTranslogUUID.equals(commitUserData.get(Translog.TRANSLOG_UUID_KEY)) == false) { return i + 1; } - // 5.x commits do not contain MAX_SEQ_NO. + // 5.x commits do not contain MAX_SEQ_NO, we should not keep it and the older commits. if (commitUserData.containsKey(SequenceNumbers.MAX_SEQ_NO) == false) { - return i; + return Math.min(commits.size() - 1, i + 1); } final long maxSeqNoFromCommit = Long.parseLong(commitUserData.get(SequenceNumbers.MAX_SEQ_NO)); if (maxSeqNoFromCommit <= globalCheckpoint) { diff --git a/server/src/test/java/org/elasticsearch/index/engine/CombinedDeletionPolicyTests.java b/server/src/test/java/org/elasticsearch/index/engine/CombinedDeletionPolicyTests.java index 0fc6195161a0a..43e3f8f64ea44 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/CombinedDeletionPolicyTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/CombinedDeletionPolicyTests.java @@ -135,15 +135,15 @@ public void testLegacyIndex() throws Exception { globalCheckpoint.set(randomLongBetween(0, maxSeqNo - 1)); indexPolicy.onCommit(Arrays.asList(legacyCommit, freshCommit)); - verify(legacyCommit, times(0)).delete(); + verify(legacyCommit, times(1)).delete(); // Do not keep the legacy commit once we have a new commit. verify(freshCommit, times(0)).delete(); - assertThat(translogPolicy.getMinTranslogGenerationForRecovery(), equalTo(legacyTranslogGen)); + assertThat(translogPolicy.getMinTranslogGenerationForRecovery(), equalTo(safeTranslogGen)); assertThat(translogPolicy.getTranslogGenerationOfLastCommit(), equalTo(safeTranslogGen)); // Make the fresh commit safe. globalCheckpoint.set(randomLongBetween(maxSeqNo, Long.MAX_VALUE)); indexPolicy.onCommit(Arrays.asList(legacyCommit, freshCommit)); - verify(legacyCommit, times(1)).delete(); + verify(legacyCommit, times(2)).delete(); verify(freshCommit, times(0)).delete(); assertThat(translogPolicy.getMinTranslogGenerationForRecovery(), equalTo(safeTranslogGen)); assertThat(translogPolicy.getTranslogGenerationOfLastCommit(), equalTo(safeTranslogGen)); From 4c686efba7be1db35be99178fc477433eaf454b3 Mon Sep 17 00:00:00 2001 From: Clinton Gormley Date: Fri, 12 Jan 2018 11:19:57 +0100 Subject: [PATCH 2/7] Fixed the cat.health REST test to accept 4ms, not just 4.0ms (#28186) --- .../main/resources/rest-api-spec/test/cat.health/10_basic.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.health/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.health/10_basic.yml index 380caec931899..504b7c8f9b1b6 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.health/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.health/10_basic.yml @@ -45,7 +45,7 @@ \d+ \s+ # init \d+ \s+ # unassign \d+ \s+ # pending_tasks - (-|\d+[.]\d+ms|s) \s+ # max task waiting time + (-|\d+(?:[.]\d+)?m?s) \s+ # max task waiting time \d+\.\d+% # active shards percent \n )+ @@ -72,7 +72,7 @@ \d+ \s+ # init \d+ \s+ # unassign \d+ \s+ # pending_tasks - (-|\d+[.]\d+ms|s) \s+ # max task waiting time + (-|\d+(?:[.]\d+)?m?s) \s+ # max task waiting time \d+\.\d+% # active shards percent \n )+ From 58d9299d49d30199424324ecc8b9638989b40eee Mon Sep 17 00:00:00 2001 From: Andrew Banchich Date: Fri, 12 Jan 2018 05:47:39 -0500 Subject: [PATCH 3/7] [Docs] Spelling fix in painless-getting-started.asciidoc (#28187) --- docs/painless/painless-getting-started.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/painless/painless-getting-started.asciidoc b/docs/painless/painless-getting-started.asciidoc index 98209fc7da9e5..155b5f272b426 100644 --- a/docs/painless/painless-getting-started.asciidoc +++ b/docs/painless/painless-getting-started.asciidoc @@ -234,7 +234,7 @@ POST hockey/player/_update_by_query ---------------------------------------------------------------- // CONSOLE -Using the match operator (`==~`) you can update all the hockey players who's +Using the match operator (`==~`) you can update all the hockey players whose names start with a consonant and end with a vowel: [source,js] From 9b9df0059fe8cec7f4e254464e8fb2468f263372 Mon Sep 17 00:00:00 2001 From: Stuart Cam Date: Fri, 12 Jan 2018 21:56:37 +1100 Subject: [PATCH 4/7] [Docs] Delete incorrect migration notes (#27915) --- docs/reference/migration/migrate_6_0/rest.asciidoc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/docs/reference/migration/migrate_6_0/rest.asciidoc b/docs/reference/migration/migrate_6_0/rest.asciidoc index c9bdc8785f1c4..9c183d4bd0c07 100644 --- a/docs/reference/migration/migrate_6_0/rest.asciidoc +++ b/docs/reference/migration/migrate_6_0/rest.asciidoc @@ -38,12 +38,6 @@ In previous versions of Elasticsearch, Analyze API is requiring a `tokenizer` or In Elasticsearch 6.0.0, Analyze API can analyze a text as a keyword field with custom normalizer or if `char_filter`/`filter` is set and `tokenizer`/`analyzer` is not set. -==== Indices exists API - -The `ignore_unavailable` and `allow_no_indices` options are no longer accepted -as they could cause undesired results when their values differed from their -defaults. - ==== `timestamp` and `ttl` in index requests `timestamp` and `ttl` are not accepted anymore as parameters of index/update From 54569afac873515cfbb6580c9582fcf3a746a186 Mon Sep 17 00:00:00 2001 From: Boudewijn <31416818+boudewijnk@users.noreply.github.com> Date: Wed, 10 Jan 2018 18:00:12 +0100 Subject: [PATCH 5/7] Update getting-started.asciidoc (#28145) Replaced single quotation marks with double quotation marks surrounding localhost address --- docs/reference/getting-started.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/reference/getting-started.asciidoc b/docs/reference/getting-started.asciidoc index 2ebe8c038655b..0a6dbd0eb8359 100755 --- a/docs/reference/getting-started.asciidoc +++ b/docs/reference/getting-started.asciidoc @@ -669,8 +669,8 @@ You can download the sample dataset (accounts.json) from https://github.com/elas [source,sh] -------------------------------------------------- -curl -H "Content-Type: application/json" -XPOST 'localhost:9200/bank/account/_bulk?pretty&refresh' --data-binary "@accounts.json" -curl 'localhost:9200/_cat/indices?v' +curl -H "Content-Type: application/json" -XPOST "localhost:9200/bank/account/_bulk?pretty&refresh" --data-binary "@accounts.json" +curl "localhost:9200/_cat/indices?v" -------------------------------------------------- // NOTCONSOLE From 19ed8491aefcc42f2dfdb8a12804b01ba6996b96 Mon Sep 17 00:00:00 2001 From: Andrew Banchich Date: Wed, 10 Jan 2018 11:59:01 -0500 Subject: [PATCH 6/7] text fixes (#28136) --- .../aggregations/pipeline/bucket-selector-aggregation.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/aggregations/pipeline/bucket-selector-aggregation.asciidoc b/docs/reference/aggregations/pipeline/bucket-selector-aggregation.asciidoc index cd0218e7c4353..5dc1b80d4adda 100644 --- a/docs/reference/aggregations/pipeline/bucket-selector-aggregation.asciidoc +++ b/docs/reference/aggregations/pipeline/bucket-selector-aggregation.asciidoc @@ -6,7 +6,7 @@ in the parent multi-bucket aggregation. The specified metric must be numeric and If the script language is `expression` then a numeric return value is permitted. In this case 0.0 will be evaluated as `false` and all other values will evaluate to true. -NOTE: The bucket_selector aggregation, like all pipeline aggregations, executions after all other sibling aggregations. This means that +NOTE: The bucket_selector aggregation, like all pipeline aggregations, executes after all other sibling aggregations. This means that using the bucket_selector aggregation to filter the returned buckets in the response does not save on execution time running the aggregations. ==== Syntax From 0c0dc3ca174a9211306928076451334d4aeda4aa Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 12 Jan 2018 16:17:30 -0500 Subject: [PATCH 7/7] Fix lock accounting in releasable lock Releasble locks hold accounting on who holds the lock when assertions are enabled. However, the underlying lock can be re-entrant yet we mark the lock as not held by the current thread as soon as the releasable is closed. For a re-entrant lock this is not right because the thread could have entered the lock multiple times. Instead, we have to count how many times the thread has entered the lock and only mark the lock as not held by the current thread when the counter reaches zero. Relates #28202 --- .../util/concurrent/ReleasableLock.java | 20 ++-- .../util/concurrent/ReleasableLockTests.java | 97 +++++++++++++++++++ 2 files changed, 111 insertions(+), 6 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/common/util/concurrent/ReleasableLockTests.java diff --git a/server/src/main/java/org/elasticsearch/common/util/concurrent/ReleasableLock.java b/server/src/main/java/org/elasticsearch/common/util/concurrent/ReleasableLock.java index 46d6abff17632..9c90b3bbde313 100644 --- a/server/src/main/java/org/elasticsearch/common/util/concurrent/ReleasableLock.java +++ b/server/src/main/java/org/elasticsearch/common/util/concurrent/ReleasableLock.java @@ -31,8 +31,9 @@ public class ReleasableLock implements Releasable { private final Lock lock; - /* a per thread boolean indicating the lock is held by it. only works when assertions are enabled */ - private final ThreadLocal holdingThreads; + + // a per-thread count indicating how many times the thread has entered the lock; only works if assertions are enabled + private final ThreadLocal holdingThreads; public ReleasableLock(Lock lock) { this.lock = lock; @@ -57,12 +58,19 @@ public ReleasableLock acquire() throws EngineException { } private boolean addCurrentThread() { - holdingThreads.set(true); + final Integer current = holdingThreads.get(); + holdingThreads.set(current == null ? 1 : current + 1); return true; } private boolean removeCurrentThread() { - holdingThreads.remove(); + final Integer count = holdingThreads.get(); + assert count != null && count > 0; + if (count == 1) { + holdingThreads.remove(); + } else { + holdingThreads.set(count - 1); + } return true; } @@ -70,7 +78,7 @@ public Boolean isHeldByCurrentThread() { if (holdingThreads == null) { throw new UnsupportedOperationException("asserts must be enabled"); } - Boolean b = holdingThreads.get(); - return b != null && b.booleanValue(); + final Integer count = holdingThreads.get(); + return count != null && count > 0; } } diff --git a/server/src/test/java/org/elasticsearch/common/util/concurrent/ReleasableLockTests.java b/server/src/test/java/org/elasticsearch/common/util/concurrent/ReleasableLockTests.java new file mode 100644 index 0000000000000..6a303449ce1c1 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/common/util/concurrent/ReleasableLockTests.java @@ -0,0 +1,97 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.util.concurrent; + +import org.elasticsearch.common.lease.Releasable; +import org.elasticsearch.test.ESTestCase; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +public class ReleasableLockTests extends ESTestCase { + + /** + * Test that accounting on whether or not a thread holds a releasable lock is correct. Previously we had a bug where on a re-entrant + * lock that if a thread entered the lock twice we would declare that it does not hold the lock after it exits its first entrance but + * not its second entrance. + * + * @throws BrokenBarrierException if awaiting on the synchronization barrier breaks + * @throws InterruptedException if awaiting on the synchronization barrier is interrupted + */ + public void testIsHeldByCurrentThread() throws BrokenBarrierException, InterruptedException { + final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + final ReleasableLock readLock = new ReleasableLock(readWriteLock.readLock()); + final ReleasableLock writeLock = new ReleasableLock(readWriteLock.writeLock()); + + final int numberOfThreads = scaledRandomIntBetween(1, 32); + final int iterations = scaledRandomIntBetween(1, 32); + final CyclicBarrier barrier = new CyclicBarrier(1 + numberOfThreads); + final List threads = new ArrayList<>(); + for (int i = 0; i < numberOfThreads; i++) { + final Thread thread = new Thread(() -> { + try { + barrier.await(); + } catch (final BrokenBarrierException | InterruptedException e) { + throw new RuntimeException(e); + } + for (int j = 0; j < iterations; j++) { + if (randomBoolean()) { + acquire(readLock, writeLock); + } else { + acquire(writeLock, readLock); + } + } + try { + barrier.await(); + } catch (final BrokenBarrierException | InterruptedException e) { + throw new RuntimeException(e); + } + }); + threads.add(thread); + thread.start(); + } + + barrier.await(); + barrier.await(); + for (final Thread thread : threads) { + thread.join(); + } + } + + private void acquire(final ReleasableLock lockToAcquire, final ReleasableLock otherLock) { + try (@SuppressWarnings("unused") Releasable outer = lockToAcquire.acquire()) { + assertTrue(lockToAcquire.isHeldByCurrentThread()); + assertFalse(otherLock.isHeldByCurrentThread()); + try (@SuppressWarnings("unused") Releasable inner = lockToAcquire.acquire()) { + assertTrue(lockToAcquire.isHeldByCurrentThread()); + assertFalse(otherLock.isHeldByCurrentThread()); + } + // previously there was a bug here and this would return false + assertTrue(lockToAcquire.isHeldByCurrentThread()); + assertFalse(otherLock.isHeldByCurrentThread()); + } + assertFalse(lockToAcquire.isHeldByCurrentThread()); + assertFalse(otherLock.isHeldByCurrentThread()); + } + +}