-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'returnDocumentResponses' for "insertMany" #1161
Merged
Merged
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
5d19a40
Start adding wiring for 'returnDocumentPositions'
tatu-at-datastax f3b0df9
mvn fmt:format
tatu-at-datastax 73645ca
...
tatu-at-datastax 2468222
Fix test signatures
tatu-at-datastax c02673a
More plumbing, get "returnDocumentPositions" where needed
tatu-at-datastax f741b3e
Test refactoring
tatu-at-datastax aa0423c
More test refactoring
tatu-at-datastax c9975d8
...
tatu-at-datastax 74b07df
Start piping through position of document
tatu-at-datastax a780120
Add new status keys, clean up ITs
tatu-at-datastax 7526ad2
More plumbing, order failed/ok insertions by input position
tatu-at-datastax 28fcb2b
...
tatu-at-datastax d99d503
Merge branch 'main' into tatu/c2-3382-return-doc-positions
tatu-at-datastax 616b34a
Fix ITs not to rely on insertion/fail order for unordered case
tatu-at-datastax 499c2e6
Merge branch 'main' into tatu/c2-3382-return-doc-positions
tatu-at-datastax 63451c1
Merge branch 'main' into tatu/c2-3382-return-doc-positions
tatu-at-datastax 009878a
Implement functionality; need tests next
tatu-at-datastax ca553ea
Add the first test for actual return-on-dups
tatu-at-datastax 6aae747
Refactoring
tatu-at-datastax 60b4c6b
Add test verifying returning of auto-generated UUIDs
tatu-at-datastax bf45bfa
Bit more testing
tatu-at-datastax c96cdb6
Merge branch 'main' into tatu/c2-3382-return-doc-positions
tatu-at-datastax 2aa4593
Merge branch 'main' into tatu/c2-3382-return-doc-positions
tatu-at-datastax 6949204
Renaming
tatu-at-datastax 91854a3
Rename setting in integration tests too (returnDocumentPositions->ret…
tatu-at-datastax 8d7d0ea
Merge branch 'main' into tatu/c2-3382-return-doc-positions
tatu-at-datastax 28b4fd0
Some scaffolding for rewrite to new spec
tatu-at-datastax 4f1db6f
Finish initial implementation
tatu-at-datastax 0c655e4
Fix 2 ITs wrt output structure changes
tatu-at-datastax adfd9fa
comment improvement
tatu-at-datastax da1aa4d
Comment improvement
tatu-at-datastax f18572c
Add comments suggested by code review
tatu-at-datastax File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,16 @@ | ||
package io.stargate.sgv2.jsonapi.service.operation.model.impl; | ||
|
||
import com.fasterxml.jackson.annotation.JsonInclude; | ||
import com.fasterxml.jackson.annotation.JsonPropertyOrder; | ||
import io.smallrye.mutiny.tuples.Tuple2; | ||
import io.stargate.sgv2.jsonapi.api.model.command.CommandResult; | ||
import io.stargate.sgv2.jsonapi.api.model.command.CommandStatus; | ||
import io.stargate.sgv2.jsonapi.exception.JsonApiException; | ||
import io.stargate.sgv2.jsonapi.exception.mappers.ThrowableToErrorMapper; | ||
import io.stargate.sgv2.jsonapi.service.shredding.model.DocumentId; | ||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.Comparator; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.function.Supplier; | ||
|
@@ -15,59 +19,121 @@ | |
* The internal to insert operation results, keeping ids of successfully and not-successfully | ||
* inserted documents. | ||
* | ||
* <p>Can serve as an aggregator, using the {@link #aggregate(DocumentId, Throwable)} function. | ||
* <p>Can serve as an aggregator, using the {@link #aggregate} function. | ||
* | ||
* @param insertedIds Documents IDs that we successfully inserted. | ||
* @param failedIds Document IDs that failed to be inserted. | ||
* @param successfulInsertions Documents that we successfully inserted. | ||
* @param failedInsertions Documents that failed to be inserted, along with failure reason. | ||
*/ | ||
public record InsertOperationPage( | ||
List<DocumentId> insertedIds, Map<DocumentId, Throwable> failedIds) | ||
List<InsertOperation.WritableDocAndPosition> allAttemptedInsertions, | ||
boolean returnDocumentResponses, | ||
List<InsertOperation.WritableDocAndPosition> successfulInsertions, | ||
List<Tuple2<InsertOperation.WritableDocAndPosition, Throwable>> failedInsertions) | ||
implements Supplier<CommandResult> { | ||
enum InsertionStatus { | ||
OK, | ||
ERROR, | ||
SKIPPED | ||
} | ||
|
||
@JsonPropertyOrder({"_id", "status", "errorsIdx"}) | ||
@JsonInclude(JsonInclude.Include.NON_NULL) | ||
record InsertionResult(DocumentId _id, InsertionStatus status, Integer errorsIdx) {} | ||
|
||
/** No-arg constructor, usually used for aggregation. */ | ||
public InsertOperationPage() { | ||
this(new ArrayList<>(), new HashMap<>()); | ||
public InsertOperationPage( | ||
List<InsertOperation.WritableDocAndPosition> allAttemptedInsertions, | ||
boolean returnDocumentResponses) { | ||
this(allAttemptedInsertions, returnDocumentResponses, new ArrayList<>(), new ArrayList<>()); | ||
} | ||
|
||
/** {@inheritDoc} */ | ||
@Override | ||
public CommandResult get() { | ||
// if we have errors, transform | ||
if (null != failedIds && !failedIds().isEmpty()) { | ||
|
||
List<CommandResult.Error> errors = new ArrayList<>(failedIds.size()); | ||
failedIds.forEach((documentId, throwable) -> errors.add(getError(documentId, throwable))); | ||
// Ensure insertions and errors are in the input order (wrt unordered insertions), | ||
// regardless of output format | ||
Collections.sort(successfulInsertions); | ||
if (!failedInsertions().isEmpty()) { | ||
Collections.sort( | ||
failedInsertions, Comparator.comparing(tuple -> tuple.getItem1().position())); | ||
} | ||
|
||
if (!returnDocumentResponses()) { // legacy output, limited to ids, error messages | ||
List<CommandResult.Error> errors; | ||
if (failedInsertions().isEmpty()) { | ||
errors = null; | ||
} else { | ||
errors = | ||
failedInsertions.stream() | ||
.map(tuple -> getError(tuple.getItem1().document().id(), tuple.getItem2())) | ||
.toList(); | ||
} | ||
// Old style, simple ids: | ||
List<DocumentId> insertedIds = | ||
successfulInsertions.stream().map(docAndPos -> docAndPos.document().id()).toList(); | ||
return new CommandResult(null, Map.of(CommandStatus.INSERTED_IDS, insertedIds), errors); | ||
} | ||
|
||
// id no errors, just inserted ids | ||
return new CommandResult(Map.of(CommandStatus.INSERTED_IDS, insertedIds)); | ||
// New style output: detailed responses. | ||
InsertionResult[] results = new InsertionResult[allAttemptedInsertions().size()]; | ||
List<CommandResult.Error> errors = new ArrayList<>(); | ||
|
||
// Results array filled in order: first successful insertions | ||
for (InsertOperation.WritableDocAndPosition docAndPos : successfulInsertions) { | ||
results[docAndPos.position()] = | ||
new InsertionResult(docAndPos.document().id(), InsertionStatus.OK, null); | ||
} | ||
// Second: failed insertions | ||
for (Tuple2<InsertOperation.WritableDocAndPosition, Throwable> failed : failedInsertions) { | ||
InsertOperation.WritableDocAndPosition docAndPos = failed.getItem1(); | ||
Throwable throwable = failed.getItem2(); | ||
CommandResult.Error error = getError(throwable); | ||
|
||
// We want to avoid adding the same error multiple times, so we keep track of the index: | ||
// either one exists, use it; or if not, add it and use the new index. | ||
int errorIdx = errors.indexOf(error); | ||
if (errorIdx < 0) { // new non-dup error; add it | ||
errorIdx = errors.size(); // will be appended at the end | ||
errors.add(error); | ||
} | ||
results[docAndPos.position()] = | ||
new InsertionResult(docAndPos.document().id(), InsertionStatus.ERROR, errorIdx); | ||
} | ||
// And third, if any, skipped insertions; those that were not attempted (f.ex due | ||
// to failure for ordered inserts) | ||
for (int i = 0; i < results.length; i++) { | ||
if (null == results[i]) { | ||
tatu-at-datastax marked this conversation as resolved.
Show resolved
Hide resolved
|
||
results[i] = | ||
new InsertionResult( | ||
allAttemptedInsertions.get(i).document().id(), InsertionStatus.SKIPPED, null); | ||
} | ||
} | ||
return new CommandResult( | ||
null, Map.of(CommandStatus.DOCUMENT_RESPONSES, Arrays.asList(results)), errors); | ||
} | ||
|
||
private static CommandResult.Error getError(DocumentId documentId, Throwable throwable) { | ||
String message = | ||
"Failed to insert document with _id %s: %s".formatted(documentId, throwable.getMessage()); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was not being used, not sure why it was there (ThrowableToErrorMapper handles this logic) |
||
Map<String, Object> fields = new HashMap<>(); | ||
fields.put("exceptionClass", throwable.getClass().getSimpleName()); | ||
if (throwable instanceof JsonApiException jae) { | ||
fields.put("errorCode", jae.getErrorCode().name()); | ||
} | ||
return ThrowableToErrorMapper.getMapperWithMessageFunction().apply(throwable, message); | ||
} | ||
|
||
private static CommandResult.Error getError(Throwable throwable) { | ||
return ThrowableToErrorMapper.getMapperWithMessageFunction() | ||
.apply(throwable, throwable.getMessage()); | ||
} | ||
|
||
/** | ||
* Aggregates the result of the insert operation into this object. | ||
* | ||
* @param id ID of the document that was inserted written. | ||
* @param failure If not null, means an error occurred during the write. | ||
* @param docWithPosition Document that was inserted (or failed to be inserted) | ||
* @param failure If not null, means an error occurred during attempted insertion | ||
*/ | ||
public void aggregate(DocumentId id, Throwable failure) { | ||
if (null != failure) { | ||
failedIds.put(id, failure); | ||
public void aggregate(InsertOperation.WritableDocAndPosition docWithPosition, Throwable failure) { | ||
if (null == failure) { | ||
successfulInsertions.add(docWithPosition); | ||
} else { | ||
insertedIds.add(id); | ||
failedInsertions.add(Tuple2.of(docWithPosition, failure)); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,9 @@ | ||
package io.stargate.sgv2.jsonapi.service.resolver.model.impl; | ||
|
||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; | ||
import io.stargate.sgv2.jsonapi.api.model.command.impl.InsertManyCommand; | ||
import io.stargate.sgv2.jsonapi.service.operation.model.Operation; | ||
import io.stargate.sgv2.jsonapi.service.operation.model.impl.InsertOperation; | ||
import io.stargate.sgv2.jsonapi.service.projection.IndexingProjector; | ||
import io.stargate.sgv2.jsonapi.service.resolver.model.CommandResolver; | ||
import io.stargate.sgv2.jsonapi.service.shredding.Shredder; | ||
import io.stargate.sgv2.jsonapi.service.shredding.model.WritableShreddedDocument; | ||
|
@@ -18,12 +16,10 @@ | |
public class InsertManyCommandResolver implements CommandResolver<InsertManyCommand> { | ||
|
||
private final Shredder shredder; | ||
private final ObjectMapper objectMapper; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was not being used, removed. |
||
|
||
@Inject | ||
public InsertManyCommandResolver(Shredder shredder, ObjectMapper objectMapper) { | ||
public InsertManyCommandResolver(Shredder shredder) { | ||
this.shredder = shredder; | ||
this.objectMapper = objectMapper; | ||
} | ||
|
||
@Override | ||
|
@@ -33,15 +29,13 @@ public Class<InsertManyCommand> getCommandClass() { | |
|
||
@Override | ||
public Operation resolveCommand(CommandContext ctx, InsertManyCommand command) { | ||
final IndexingProjector projection = ctx.indexingProjector(); | ||
final List<WritableShreddedDocument> shreddedDocuments = | ||
command.documents().stream().map(doc -> shredder.shred(ctx, doc, null)).toList(); | ||
|
||
// resolve ordered | ||
InsertManyCommand.Options options = command.options(); | ||
boolean ordered = (null != options) && options.ordered(); | ||
boolean returnDocumentResponses = (null != options) && options.returnDocumentResponses(); | ||
|
||
boolean ordered = null != options && Boolean.TRUE.equals(options.ordered()); | ||
|
||
return new InsertOperation(ctx, shreddedDocuments, ordered); | ||
return new InsertOperation(ctx, shreddedDocuments, ordered, false, returnDocumentResponses); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this so that both
null
and[ ]
values are skipped (not just nulls)