Skip to content

Unify partial-result persistence with the final ScanResult write path.#74

Open
MFormenti wants to merge 2 commits into
mainfrom
unify-partial-result-persistence
Open

Unify partial-result persistence with the final ScanResult write path.#74
MFormenti wants to merge 2 commits into
mainfrom
unify-partial-result-persistence

Conversation

@MFormenti

Copy link
Copy Markdown

Both partial and final results now go through JacksonMongoCollection keyed by the job ID, so the final result overwrites the partial in place instead of leaving an orphan RUNNING document written via a separate raw-collection path.

Both partial and final results now go through JacksonMongoCollection<ScanResult> keyed by the job ID, so the final result overwrites the partial in place instead of leaving an orphan RUNNING document written via a separate raw-collection path.
// Reuse the same write path (JacksonMongoCollection<ScanResult> upsert) as final
// results so partial and final results share serialization logic and the same _id.
// A failed partial write must not abort the scan, hence the swallowed exception.
writeResultToDatabase(dbName, collectionName, partialResult);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I think we should not write directly to the database. The reason is because a CANCELLED-state result can be overwritten by a RUNNING-state-partial result. If this happens we will have the impression that nothing wrong happened during the scan when what we will see in the database is a reverted final state. This can happens for example when the thread containing the method waitForScanResult write a CANCELLED state into mongo, due to a timeout, while a probe was in the middle of its process to write a status RUNNING. The status RUNNING will be a forever status in this case since isDone will be true.

Recommendation is replace line 420 with

        resultCollectionCache
                .getUnchecked(Pair.of(dbName, collectionName))
                .replaceOne(
                        new org.bson.Document("_id", partialResult.getId())
                                .append("resultStatus", JobStatus.RUNNING.toString()),
                        partialResult,
                        new ReplaceOptions().upsert(true));

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I didn't think of the multi-thread conflict scenario.
I implemented the suggested fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants