291 refactor datastore callables part 3#356
Conversation
| private final List<DocumentRevision> results; | ||
|
|
||
| protected Changes(long lastSequence, List<DocumentRevision> results) { | ||
| public Changes(long lastSequence, List<DocumentRevision> results) { |
There was a problem hiding this comment.
Perhaps add a comment indicating that API users are not expected to construct this and should call com.cloudant.sync.datastore.Datastore#changes since it is now publicly visible.
| InsertRevisionCallable insertRevisionCallable = this.insertNewWinnerRevisionAdaptor(body, preRevision); | ||
| String newRevisionId = insertRevisionCallable.revId; | ||
| insertRevisionCallable.call(db); | ||
| return new GetDocumentCallable(docId, newRevisionId, this.attachmentsDir, this.attachmentStreamFactory).call(db); |
There was a problem hiding this comment.
Should this be a TODO as well to build the new DocumentRevision instead of retrieving the whole document again?
There was a problem hiding this comment.
See 23ce739 - we can sweep these all up into a ticket/gist to use in a later PR
| } | ||
| // before starting the tx, get the 'new winner' and see if we need to prepare its | ||
| // attachments | ||
| DocumentRevisionTree docTree = queue.submit(new GetAllRevisionsOfDocumentCallable(docId, attachmentsDir, attachmentStreamFactory)).get(); |
There was a problem hiding this comment.
Before all parts of this resolution were in a single transaction. Do you think it is safe to do it in parts? I guess the revisions of the document could change between this get and the later resolve and update of attachments.
There was a problem hiding this comment.
Thanks for spotting, restored in 78a8c67
There was a problem hiding this comment.
The first getAllRevisionsOfDocumentCallable is still not part of the tx, was that oversight or intentional?
There was a problem hiding this comment.
Oh I was following the comment but yes in the original version it was inside the transaction. It possibly doesn't make much/any difference because getAllRevisionsOfDocumentCallable doesn't modify the database, but I could put it back in the transaction to be consistent with the original code.
There was a problem hiding this comment.
Yeah considering the scenarios:
rev1,rev2<-getAllRevisionsOfDocumentCallable- New
rev3inserted - Transaction
- Resolve conflicts between
rev1andrev2
- Resolve conflicts between
- Conflict exists with
rev3
vs
- Transaction
rev1,rev2<-getAllRevisionsOfDocumentCallable- Resolve conflicts between
rev1andrev2
- New
rev3inserted - Conflict exists with
rev3
The outcome isn't different even though it seems like the second case provides some protection against a new conflict being created during the resolution so you're right there probably isn't anything further to be gained.
| existingAttachments, attachmentsDir, attachmentStreamFactory)).get(); | ||
| } | ||
| } catch (AttachmentException e) { | ||
| // TODO |
There was a problem hiding this comment.
Think we should at least add a log message here before merging even if a more robust solution is intended in a later pass.
There was a problem hiding this comment.
Due to changes in 78a8c67 this should no longer be present
| } | ||
|
|
||
| private String insertNewWinnerRevision(SQLDatabase db, DocumentBody newWinner, | ||
| // TODO should this be a callable itself? |
There was a problem hiding this comment.
I don't think so because it doesn't run against the DB, which I think was the pattern for the callables.
| } | ||
|
|
||
| /** | ||
| * @return Whether the body has been modified |
There was a problem hiding this comment.
modified since when?
DocumentRevision is api_public so we should try to be clear about what the method does or is for or possibly just say it is for internal use.
| public List<DocumentRevision> call(SQLDatabase db) throws Exception { | ||
| String sql = String.format("SELECT " + DatastoreImpl.FULL_DOCUMENT_COLS + " FROM revs, docs" + | ||
| " WHERE docid IN ( %1$s ) AND current = 1 AND docs.doc_id = revs" + | ||
| ".doc_id " + |
There was a problem hiding this comment.
formatting is a bit weird here
tomblench
force-pushed
the
291-refactor-datastore-callables-part-3
branch
2 times, most recently
from
5d7aa9b to
2c4c991
Compare
Almost everything else is now in callables except `AttachmentManager` code.
tomblench
force-pushed
the
291-refactor-datastore-callables-part-3
branch
from
ea7a55f to
d36f18c
Compare
2 participants
Everything except AttachmentManager into callables
TODO
Check exception correctnessDatastoreImplsignatures haven't changed so they still throw the same exceptions. See Refactor - fix exception handling in Callables #354 and Unchecked exception audit #280 - these are the correct follow on PRs to use to address the other exception issuesFix upcatch(InterruptedException e)/catch (ExecutionException e)pattern inDatastoreImpljavadoc commentsCopyright headersRun some proper replication tests- pull and pull replication looks good with the Airfi dataset