Add support for append only indices #17039
Conversation
|
❌ Gradle check result for 38061f7: FAILURE 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? |
RS146BIJAY
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
jainankitk and
linuxpi
as code owners
RS146BIJAY
changed the title
Signed-off-by: RS146BIJAY <[email protected]>
RS146BIJAY
force-pushed
the
append-only-indices
branch
from
38061f7 to
7af2713
Compare
|
❌ Gradle check result for 7af2713: FAILURE 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? |
| * | ||
| * @opensearch.internal | ||
| */ | ||
| public class IndexingRetryException extends RuntimeException { |
There was a problem hiding this comment.
Lets extend from OpenSearchException
| @@ -584,7 +584,7 @@ public static IndexMergePolicy fromString(String text) { | |||
| */ | |||
| public static final Setting<Boolean> INDEX_UNREFERENCED_FILE_CLEANUP = Setting.boolSetting( | |||
| "index.unreferenced_file_cleanup.enabled", | |||
| true, | |||
| false, | |||
There was a problem hiding this comment.
Non intentional change. Will correct it.
| SETTING_INDEX_APPEND_ONLY_ENABLED, | ||
| false, | ||
| Property.IndexScope, | ||
| Property.Final |
There was a problem hiding this comment.
Should we allow it to be changed from true to false at some later point in the future? I'm ok to have it as final for now but wondering if there are use-cases which require such support.
There was a problem hiding this comment.
Actually true to false can be supported. But false to true should be blocked
| @@ -538,6 +539,11 @@ protected void doRun() { | |||
| if (docWriteRequest == null) { | |||
| continue; | |||
| } | |||
|
|
|||
| if (addFailureIfIndexAppendOnlyAndOpsDeleteOrUpdate(docWriteRequest, i, concreteIndices, metadata)) { | |||
There was a problem hiding this comment.
Should this be checked for only after confirming index is not available through addFailureIfIndexIsUnavailable?
This will help short circuit for index unavailable cases faster.
| } | ||
|
|
||
| final IndexMetadata indexMetadata = metadata.index(concreteIndex); | ||
| if (indexMetadata.isAppendOnlyIndex() == true) { |
| } else if (request.id() != null && request.opType() == DocWriteRequest.OpType.INDEX) { | ||
| ValidationException exception = new ValidationException(); | ||
| exception.addValidationError( | ||
| "Operation [" + request.opType() + "] is not allowed with a custom doc id " + request.id() |
There was a problem hiding this comment.
Can we add to the message that the reason validation is failing because append only setting is set to true?
| if ((request.opType() == DocWriteRequest.OpType.UPDATE || request.opType() == DocWriteRequest.OpType.DELETE)) { | ||
| ValidationException exception = new ValidationException(); | ||
| exception.addValidationError( | ||
| "Operation [" + request.opType() + "] is not allowed for append only index " + request.index() |
There was a problem hiding this comment.
Can we add to the message that the reason validation is failing because append only setting is set to true?
| @@ -756,6 +762,44 @@ public void onTimeout(TimeValue timeout) { | |||
| }); | |||
| } | |||
|
|
|||
| private boolean addFailureIfIndexAppendOnlyAndOpsDeleteOrUpdate( | |||
There was a problem hiding this comment.
Can you add UTs for this in TransportBulkActionTests?
| response = client().prepareBulk().add(client().prepareUpdate("index", "0").setDoc("foo", "updated")).get(); | ||
| assertThat( | ||
| response.getItems()[0].getFailureMessage(), | ||
| containsString("Operation [UPDATE] is not allowed for append only index index;") | ||
| ); | ||
|
|
||
| response = client().prepareBulk().add(client().prepareDelete("index", "0")).get(); | ||
| assertThat( | ||
| response.getItems()[0].getFailureMessage(), | ||
| containsString("Operation [DELETE] is not allowed for append only index index;") | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why are we testing update/delete in retry test?
There was a problem hiding this comment.
These calls will be blocked at the transport layer itself. I was thinking around covering different scenarios. Will remove this.
Description
Adding support for Append only indices where we are disabling any form of updates and deletes on the index. We are also preventing passing a custom doc id for bulk indexing requests. Append only indices will be supported through a configurable setting. If this setting is enabled, all update and delete operations on the index (UPDATE, DELETE, UPSERT, UPDATE BY QUERY, DELETE BY QUERY, etc,) will be blocked.
Related Issues
#12886
Check List
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.