[Searchable Snapshot] Use java.lang.ref.Cleaner to close cloned IndexInputs #6351

andrross · 2023-02-17T01:05:53Z

As detailed in this issue, Lucene does not close the cloned IndexInput instances, so we are using the Cleaner mechanism from the JDK to close any unclosed clones.

A single static Cleaner instance to ensure any unclosed clone of an IndexInput is closed. This instance creates a single daemon thread on which it performs the cleaning actions. For an already-closed IndexInput, the cleaning action is a no-op. For an open IndexInput, the close action will decrement a reference count.

Issues Resolved

Closes #5243

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

andrross · 2023-02-17T01:08:25Z

...c/test/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInputTests.java

@@ -301,12 +304,10 @@ public static void testSlice(OnDemandBlockSnapshotIndexInput blockedSnapshotFile

            newSlice.seek(0);
            assertEquals(0, newSlice.getFilePointer());
-            assertEquals(0, newSlice.currentBlockId);


Note that I did remove a couple assertions around currentBlockId because I wanted to make that field private instead of protected. We're not losing any coverage here because computing currentBlockPosition() requires the currentBlockId so we still (indirectly) asserting on the currentBlockId value.

reta · 2023-02-17T01:22:02Z

server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockIndexInput.java

+     * [1]: https://github.com/apache/lucene/blob/8340b01c3cc229f33584ce2178b07b8984daa6a9/lucene/core/src/java/org/apache/lucene/store/IndexInput.java#L32-L33
+     */
+    private static class BlockHolder {
+        private IndexInput block;


Shouldn't block be declared volatile? it is cleaned in cleaner's thread but will be queried in other threads ...

The cleaner won't touch it until the reference is phantom reachable. I guess I was assuming there would not be any concurrency issues, but maybe that's not a valid assumption. There obviously won't be anything else reading or writing when the cleaner action is running. But I don't actually know if a memory barrier is involved so would the cleaner thread be guaranteed to see the latest value of block? I can make it volatile to be safe.

Trying to come up with any other possible scenario when close() is called but not by cleaner, probably should never happen

reta · 2023-02-17T01:26:28Z

server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockIndexInput.java

+            }
+        }
+
+        private void closeQuietly() {


IOUtils::closeWhileHandlingException

Is the logging statement I added here on failure useful?

Not sure to be fair, there is no way to propagate it ... however it seems like Cleaner keeps references on logger as well :) What we could do probably is to thrown UncheckedIOException instead

The Cleaner will silently swallow any unhandled exceptions. I think the logger reference is fine because its a static field and not tied to an instance.

FYI, the JDK's cleaner implementations do throw UncheckedIOException in this case, no logging, just a note

reta · 2023-02-17T01:27:13Z

server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockIndexInput.java

+            this.block = Objects.requireNonNull(block);
+        }
+
+        private void close() throws IOException {


It looks like AutoCloseable, right?

github-actions · 2023-02-17T01:28:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11272/
CommitID: c55f61e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

reta · 2023-02-17T01:30:48Z

server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockIndexInput.java

@@ -144,56 +146,56 @@ public long length() {

    @Override
    public byte readByte() throws IOException {
-        if (currentBlock == null) {
+        if (blockHolder.block == null) {


There could be a race (fe between readByte() and close()) when blockHolder.block becomes null after the check, the safer way would be:

final IndexInput block = blockHolder.block; if (block == null) { // seek to the beginning seek(0); } else if (currentBlockPosition() >= blockSize) { int blockId = currentBlockId + 1; demandBlock(blockId); } return block.readByte();

This class (like org.apache.lucene.store.IndexInput) is not thread safe (I'll add it to the class doc). The cleaning action is technically on a separate thread, but also guaranteed to happen when the instance can no longer be used so there's no chance of it racing with any of these methods. I agree your suggestion is the right way to handle a concurrency issue like this, but probably not necessary here. What do you think?

This class (like org.apache.lucene.store.IndexInput) is not thread safe (I'll add it to the class doc).

Yes, should be enough to clarify, thank you

github-actions · 2023-02-17T01:52:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11274/
CommitID: b8463e2

codecov-commenter · 2023-02-17T01:55:15Z

Codecov Report

Merging #6351 (e9e3646) into main (7914c04) will increase coverage by 0.04%.
The diff coverage is 61.40%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6351      +/-   ##
============================================
+ Coverage     70.73%   70.78%   +0.04%     
- Complexity    59024    59061      +37     
============================================
  Files          4800     4799       -1     
  Lines        282453   282432      -21     
  Branches      40718    40716       -2     
============================================
+ Hits         199799   199924     +125     
+ Misses        66259    66140     -119     
+ Partials      16395    16368      -27

Impacted Files	Coverage Δ
...tore/remote/directory/RemoteSnapshotDirectory.java	`3.12% <0.00%> (ø)`
...arch/index/store/remote/utils/TransferManager.java	`3.03% <ø> (ø)`
...dex/store/remote/file/OnDemandBlockIndexInput.java	`79.13% <61.81%> (+6.12%)`	⬆️
...e/remote/file/OnDemandBlockSnapshotIndexInput.java	`72.34% <100.00%> (+0.91%)`	⬆️
...search/indices/recovery/RecoveryTargetHandler.java	`0.00% <0.00%> (-100.00%)`	⬇️
...h/action/ingest/SimulateDocumentVerboseResult.java	`60.71% <0.00%> (-39.29%)`	⬇️
...cluster/coordination/PublishClusterStateStats.java	`33.33% <0.00%> (-37.51%)`	⬇️
.../opensearch/client/indices/CloseIndexResponse.java	`56.25% <0.00%> (-35.00%)`	⬇️
...ion/admin/cluster/node/info/PluginsAndModules.java	`53.12% <0.00%> (-34.38%)`	⬇️
...arch/search/aggregations/pipeline/LinearModel.java	`23.07% <0.00%> (-30.77%)`	⬇️
... and 496 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

As detailed [in this issue][1], Lucene does not close the cloned IndexInput instances, so we are using the Cleaner mechanism from the JDK to close any unclosed clones. A single static Cleaner instance to ensure any unclosed clone of an IndexInput is closed. This instance creates a single daemon thread on which it performs the cleaning actions. For an already-closed IndexInput, the cleaning action is a no-op. For an open IndexInput, the close action will decrement a reference count. [1]: opensearch-project#5243 (comment) Signed-off-by: Andrew Ross <andrross@amazon.com>

github-actions · 2023-02-17T02:31:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11277/
CommitID: e9e3646

andrross · 2023-02-17T18:02:54Z

WhiteSource is complaining about #5576 still, though no dependencies changed in this PR.

As detailed [in this issue][1], Lucene does not close the cloned IndexInput instances, so we are using the Cleaner mechanism from the JDK to close any unclosed clones. A single static Cleaner instance to ensure any unclosed clone of an IndexInput is closed. This instance creates a single daemon thread on which it performs the cleaning actions. For an already-closed IndexInput, the cleaning action is a no-op. For an open IndexInput, the close action will decrement a reference count. [1]: #5243 (comment) Signed-off-by: Andrew Ross <andrross@amazon.com> (cherry picked from commit dae1566) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

As detailed [in this issue][1], Lucene does not close the cloned IndexInput instances, so we are using the Cleaner mechanism from the JDK to close any unclosed clones. A single static Cleaner instance to ensure any unclosed clone of an IndexInput is closed. This instance creates a single daemon thread on which it performs the cleaning actions. For an already-closed IndexInput, the cleaning action is a no-op. For an open IndexInput, the close action will decrement a reference count. [1]: #5243 (comment) (cherry picked from commit dae1566) Signed-off-by: Andrew Ross <andrross@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

andrross added the skip-changelog label Feb 17, 2023

andrross requested review from reta, anasalkouz, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners February 17, 2023 01:05

andrross changed the title ~~Use java.lang.ref.Cleaner to close cloned IndexInputs~~ [Searchable Snapshot] Use java.lang.ref.Cleaner to close cloned IndexInputs Feb 17, 2023

andrross commented Feb 17, 2023

View reviewed changes

reta reviewed Feb 17, 2023

View reviewed changes

andrross force-pushed the index-input-cleaner branch from c55f61e to b8463e2 Compare February 17, 2023 01:22

reta reviewed Feb 17, 2023

View reviewed changes

andrross force-pushed the index-input-cleaner branch from b8463e2 to e9e3646 Compare February 17, 2023 01:58

andrross added backport 2.x Backport to 2.x branch v2.6.0 'Issues and PRs related to version v2.6.0' labels Feb 17, 2023

reta approved these changes Feb 17, 2023

View reviewed changes

andrross merged commit dae1566 into opensearch-project:main Feb 17, 2023

andrross deleted the index-input-cleaner branch February 17, 2023 18:03

opensearch-trigger-bot bot mentioned this pull request Feb 17, 2023

[Backport 2.x] [Searchable Snapshot] Use java.lang.ref.Cleaner to close cloned IndexInputs #6358

Merged

linuxpi mentioned this pull request Feb 21, 2023

[Remote Store] Add header(codec) and footer(checksum) to remote store segment metadata #5917

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Searchable Snapshot] Use java.lang.ref.Cleaner to close cloned IndexInputs #6351

[Searchable Snapshot] Use java.lang.ref.Cleaner to close cloned IndexInputs #6351

andrross commented Feb 17, 2023

andrross Feb 17, 2023

reta Feb 17, 2023

andrross Feb 17, 2023

reta Feb 17, 2023

reta Feb 17, 2023

andrross Feb 17, 2023

reta Feb 17, 2023

andrross Feb 17, 2023

reta Feb 17, 2023

reta Feb 17, 2023

reta Feb 17, 2023

github-actions bot commented Feb 17, 2023

reta Feb 17, 2023 •

edited

Loading

andrross Feb 17, 2023

reta Feb 17, 2023

github-actions bot commented Feb 17, 2023

codecov-commenter commented Feb 17, 2023 •

edited

Loading

github-actions bot commented Feb 17, 2023

andrross commented Feb 17, 2023

[Searchable Snapshot] Use java.lang.ref.Cleaner to close cloned IndexInputs #6351

[Searchable Snapshot] Use java.lang.ref.Cleaner to close cloned IndexInputs #6351

Conversation

andrross commented Feb 17, 2023

Issues Resolved

Check List

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2023

Gradle Check (Jenkins) Run Completed with:

reta Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Feb 17, 2023 • edited Loading

Codecov Report

github-actions bot commented Feb 17, 2023

Gradle Check (Jenkins) Run Completed with:

andrross commented Feb 17, 2023

reta Feb 17, 2023 •

edited

Loading

codecov-commenter commented Feb 17, 2023 •

edited

Loading