relates to #178: insert commands per spec, added oerdered/unordered, better tests #216
Conversation
| if (null != failedIds && !failedIds().isEmpty()) { | ||
|
|
||
| List<CommandResult.Error> errors = new ArrayList<>(failedIds.size()); | ||
| failedIds.forEach((documentId, throwable) -> errors.add(getError(documentId, throwable))); |
There was a problem hiding this comment.
currently for each failed document, I return an error, should we aggregate? if yes how?
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/InsertOperation.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/InsertOperation.java
Show resolved
Hide resolved
| WritableShreddedDocument failed = failFastInsertException.document; | ||
|
|
||
| // collect inserted, since it's sequential iterate until failed index | ||
| int failedIndex = documents().lastIndexOf(failed); |
There was a problem hiding this comment.
Do we need this logic, since we use concatenate is already ordered and status can be captured with invoke() similar to unordered.
InsertOperationPage response = new InsertOperationPage();
insertDocument(queryExecutor, query, doc) .onItemOrFailure() .invoke( (documentId, error) -> { response.aggregate(doc.id(), error); }))
There was a problem hiding this comment.
Hmm but how can I reduce? Where is the response coming from?
There was a problem hiding this comment.
@ivansenic We can do collect().last()
InsertOperationPage response = new InsertOperationPage(); return Multi.createFrom() .items(documents.stream()) .onItem() .transformToUniAndConcatenate( doc -> insertDocument(queryExecutor, query, doc) .onItemOrFailure() .invoke( (documentId, error) -> { response.aggregate(doc.id(), error); })) .collect() .last() .onItemOrFailure() .transform( (id, error) -> { return response; });
There was a problem hiding this comment.
Hmm, I don't like those side effects, especially when I am using a non-thread-safe structure.. Let me see if I can optimize this..
There was a problem hiding this comment.
@maheshrajamani this is the bug from the demo, I can not use last index on a document, as if first fails, I would assume that the first was written.. Need to change this :)
| .in(InsertOperationPage::new, (agg, in) -> agg.aggregate(in.getItem1().id(), in.getItem2())) | ||
|
|
||
| // use object identity to resolve to Supplier<CommandResult> | ||
| .map(i -> i); |
There was a problem hiding this comment.
Why is this needed in() would have already return Uni< InsertOperationPage>
There was a problem hiding this comment.
Does not compile. The method signature must be
Uni<? extends Supplier<CommandResult>>
so that we can return the Uni<InsertOperationPage>.
I wanted to refactor, but it's not a small change, should be changed in all sub-classes and tests.. Do you think it's worth it?
|
@maheshrajamani can you answer to the questions I posted? |
…odes, improved tests
ivansenic
force-pushed
the
ise-178-insert-commands
branch
from
46aa965 to
08a3092
Compare
What this PR does:
Adaptation to spec for the
insertOne&insertMany, including test improvements.insertOnecommand as per spec no options existsorderedoption for theinsertManyWhich issue(s) this PR fixes:
Relates to #178.