Replace version with reader cache key in IndicesRequestCache #34189

dnhatn · 2018-10-01T16:30:07Z

Today we use the version of a DirectoryReader as a component of the key
of IndicesRequestCache. This usage is perfectly fine since the version
is advanced every time a new change is made into IndexWriter. In other
words, two DirectoryReaders with the same version should have the same
content. However, this invariant is only guaranteed in the context of a
single IndexWriter because the version is reset to the committed version
value when IndexWriter is re-opened.

Since #33473, each IndexShard may have more than one IndexWriter, and
using the version of a DirectoryReader as a part of the cache key can
cause IndicesRequestCache to return stale cached values. For example, in
#27650, we rollback the engine (i.e., re-open IndexWriter), index new
documents, refresh, then make a count request, but the search layer
mistakenly returns the count of the DirectoryReader of the previous
IndexWriter because the current DirectoryReader has the same version of
the old DirectoryReader even their documents are different. This is
possible because these two readers come from different IndexWriters.

This commit replaces the the version with the reader cache key of
IndexReader as a component of the cache key of IndicesRequestCache.

Closes #27650
Relates #33473

/cc @ywelsch

Today we use the version of a DirectoryReader as a component of the key of IndicesRequestCache. This usage is perfectly fine since the version is advanced every time a new change is made into IndexWriter. In other words, two DirectoryReader with the same version should have the same content. However, this invariant is only guaranteed in the context of a single IndexWriter because the version is reset to the committed version value when IndexWriter is re-opened. Since elastic#33473, each IndexShard may have more than one IndexWriter, and using the version of a DirectoryReader as a part of the cache key can cause IndicesRequestCache to return stale cached values. For example, in elastic#27650, we rollback the engine (i.e., re-open IndexWriter), index new documents, refresh, then make a count request, but the search layer mistakenly returns the count of the DirectoryReader of the previous IndexWriter because the current DirectoryReader has the same version to the old DirectoryReader. This is possible because these two readers come from different IndexWriters. This commit replaces the the version with the reader cache key of IndexReader as a component of the cache key of IndicesRequestCache. Closes elastic#27650 Relates elastic#33473

elasticmachine · 2018-10-01T16:30:10Z

Pinging @elastic/es-search-aggs

dnhatn · 2018-10-01T16:30:41Z

Chatted with Boaz on another channel, I'll work on these follow-ups:

Index different number of documents in testRecoveryWithConcurrentIndexing
Make sure the global checkpoint is always initialized (never be unassigned)

jimczi

The changes look good to me. Though I wonder why we were using getVersion until now. I don't have the full history so maybe @s1monw can explain if this was needed for something non-obvious.
One reason I can think of is that the reader cache key changes every time you open a new directory reader even if nothing changed in the index but I don't see where this could be an issue since we reopen the directory reader only if something has changed.

jimczi · 2018-10-03T15:45:53Z

server/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java

@@ -137,7 +139,9 @@ BytesReference getOrCompute(CacheEntity cacheEntity, Supplier<BytesReference> lo
     * @param cacheKey the cache key to invalidate
     */
    void invalidate(CacheEntity cacheEntity, DirectoryReader reader, BytesReference cacheKey) {
-        cache.invalidate(new Key(cacheEntity, reader.getVersion(), cacheKey));
+        if (reader.getReaderCacheHelper() != null) {


Why is it possible to have a null cache helper here ?

some reader wrappers will return null here if they change the live-docs of a reader for instance. But I wonder in what case that happens?

I've replaced this check with an assertion as we should always have readerCacheKey.

s1monw

change LGTM. I think the version was really just legacy. I am not sure why we used this. I also can't think of a reason why this change would cause issues.

s1monw · 2018-10-03T18:37:12Z

server/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java

@@ -137,7 +139,9 @@ BytesReference getOrCompute(CacheEntity cacheEntity, Supplier<BytesReference> lo
     * @param cacheKey the cache key to invalidate
     */
    void invalidate(CacheEntity cacheEntity, DirectoryReader reader, BytesReference cacheKey) {
-        cache.invalidate(new Key(cacheEntity, reader.getVersion(), cacheKey));
+        if (reader.getReaderCacheHelper() != null) {


some reader wrappers will return null here if they change the live-docs of a reader for instance. But I wonder in what case that happens?

dnhatn · 2018-10-04T01:02:47Z

Thanks @jimczi and @s1monw for reviewing.

Today we use the version of a DirectoryReader as a component of the key of IndicesRequestCache. This usage is perfectly fine since the version is advanced every time a new change is made into IndexWriter. In other words, two DirectoryReaders with the same version should have the same content. However, this invariant is only guaranteed in the context of a single IndexWriter because the version is reset to the committed version value when IndexWriter is re-opened. Since #33473, each IndexShard may have more than one IndexWriter, and using the version of a DirectoryReader as a part of the cache key can cause IndicesRequestCache to return stale cached values. For example, in #27650, we rollback the engine (i.e., re-open IndexWriter), index new documents, refresh, then make a count request, but the search layer mistakenly returns the count of the DirectoryReader of the previous IndexWriter because the current DirectoryReader has the same version of the old DirectoryReader even their documents are different. This is possible because these two readers come from different IndexWriters. This commit replaces the the version with the reader cache key of IndexReader as a component of the cache key of IndicesRequestCache. Closes #27650 Relates #33473

* master: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* rename-ccr-stats: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

Today we use the version of a DirectoryReader as a component of the key of IndicesRequestCache. This usage is perfectly fine since the version is advanced every time a new change is made into IndexWriter. In other words, two DirectoryReaders with the same version should have the same content. However, this invariant is only guaranteed in the context of a single IndexWriter because the version is reset to the committed version value when IndexWriter is re-opened. Since #33473, each IndexShard may have more than one IndexWriter, and using the version of a DirectoryReader as a part of the cache key can cause IndicesRequestCache to return stale cached values. For example, in #27650, we rollback the engine (i.e., re-open IndexWriter), index new documents, refresh, then make a count request, but the search layer mistakenly returns the count of the DirectoryReader of the previous IndexWriter because the current DirectoryReader has the same version of the old DirectoryReader even their documents are different. This is possible because these two readers come from different IndexWriters. This commit replaces the the version with the reader cache key of IndexReader as a component of the cache key of IndicesRequestCache. Closes #27650 Relates #33473

dnhatn added >enhancement :Search/Search Search-related issues that do not fall into other categories v7.0.0 v6.5.0 labels Oct 1, 2018

dnhatn requested review from jpountz, s1monw and bleskes October 1, 2018 16:30

jimczi reviewed Oct 3, 2018

View reviewed changes

s1monw approved these changes Oct 3, 2018

View reviewed changes

Merge branch 'master' into cache-key

b73fbf8

dnhatn merged commit 6dd716b into elastic:master Oct 4, 2018

dnhatn deleted the cache-key branch October 4, 2018 01:03

dnhatn added the backport pending label Oct 4, 2018

dnhatn removed the backport pending label Oct 4, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace version with reader cache key in IndicesRequestCache #34189

Replace version with reader cache key in IndicesRequestCache #34189

dnhatn commented Oct 1, 2018 •

edited

Loading

elasticmachine commented Oct 1, 2018

dnhatn commented Oct 1, 2018

jimczi left a comment

jimczi Oct 3, 2018

s1monw Oct 3, 2018

dnhatn Oct 3, 2018

s1monw left a comment

s1monw Oct 3, 2018

dnhatn commented Oct 4, 2018

Replace version with reader cache key in IndicesRequestCache #34189

Replace version with reader cache key in IndicesRequestCache #34189

Conversation

dnhatn commented Oct 1, 2018 • edited Loading

elasticmachine commented Oct 1, 2018

dnhatn commented Oct 1, 2018

jimczi left a comment

Choose a reason for hiding this comment

jimczi Oct 3, 2018

Choose a reason for hiding this comment

s1monw Oct 3, 2018

Choose a reason for hiding this comment

dnhatn Oct 3, 2018

Choose a reason for hiding this comment

s1monw left a comment

Choose a reason for hiding this comment

s1monw Oct 3, 2018

Choose a reason for hiding this comment

dnhatn commented Oct 4, 2018

dnhatn commented Oct 1, 2018 •

edited

Loading