Fix race condition in SqsWorker when acknowledgements are enabled #3001

kkondaka · 2023-07-11T07:29:33Z

Description

Fix race condition in SqsWorker when acknowledgements are enabled
When E2E acknowledgements are enabled, S3 Source need to synchronize between the callback thread and the main thread because main thread may not have got chance to fully populate the waitingForAcknowledgements list

Resolves #3000

Issues Resolved

#3000

Check List

[ X] New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
[ X] Commits are signed with a real name per the DCO

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.

Signed-off-by: Krishna Kondaka <[email protected]>

engechas

Builds failing. Code LGTM minus 1 minor comment

engechas · 2023-07-11T15:42:38Z

...per-plugins/s3-source/src/main/java/org/opensearch/dataprepper/plugins/source/SqsWorker.java

@@ -226,6 +227,7 @@ && isEventBridgeEventTypeCreated(parsedMessage)) {
        for (ParsedMessage parsedMessage : parsedMessagesToRead) {
            List<DeleteMessageBatchRequestEntry> waitingForAcknowledgements = new ArrayList<>();
            AcknowledgementSet acknowledgementSet = null;
+            AtomicBoolean acknowledgementSetReady = new AtomicBoolean(false);


Doesn't look like this is used anywhere

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable · 2023-07-11T17:39:38Z

...ore/src/main/java/org/opensearch/dataprepper/acknowledgements/DefaultAcknowledgementSet.java

@@ -84,6 +86,8 @@ public boolean isDone() {
            if (Instant.now().isAfter(expiryTime)) {
                if (callbackFuture != null) {
                    callbackFuture.cancel(true);
+                    callbackFuture = null;
+                    LOG.warn("AcknowledgementSet expired");


Is there any other data we can provide to make this useful if it happens? I'd imagine there may be a lot of these and they could be hard to understand.

Perhaps include the expiry time? Could we include the overall time?

I am expecting there shouldn't be a lot of these. Since we only support one source, the acknowledgement set and expiry time should be obvious. We could pass a name to AcknowledgementSetManager.create when we support multiple sources.

dlvenable · 2023-07-11T18:32:10Z

...3-source/src/integrationTest/java/org/opensearch/dataprepper/plugins/source/SqsWorkerIT.java

+        }
+        try {
+            Thread.sleep(10000);
+        } catch (Exception e){}


Are you expecting the thread to be interrupted? If you are not expecting this, then you can just let an interruption fail the test.

Thread.sleep(10000);

and update the method to throws Exception.

Signed-off-by: Krishna Kondaka <[email protected]>

opensearch-trigger-bot · 2023-07-11T22:10:57Z

The backport to 2.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.3 2.3
# Navigate to the new working tree
cd .worktrees/backport-2.3
# Create a new branch
git switch --create backport/backport-3001-to-2.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 515cf6114f5270f5f4fc94eba6bdd62e45659944
# Push it to GitHub
git push --set-upstream origin backport/backport-3001-to-2.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.3

Then, create a pull request where the base branch is 2.3 and the compare/head branch is backport/backport-3001-to-2.3.

…ensearch-project#3001) * Fix race condition in SqsWorker when acknowledgements are enabled Signed-off-by: Krishna Kondaka <[email protected]> * Modified to do the synchronization in the acknowledgement set framework Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Removed unused variable Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comment and fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Fixed checkStyle failure Signed-off-by: Krishna Kondaka <[email protected]> --------- Signed-off-by: Krishna Kondaka <[email protected]> Co-authored-by: Krishna Kondaka <[email protected]> (cherry picked from commit 515cf61)

) (#3010) * Fix race condition in SqsWorker when acknowledgements are enabled Signed-off-by: Krishna Kondaka <[email protected]> * Modified to do the synchronization in the acknowledgement set framework Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Removed unused variable Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comment and fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Fixed checkStyle failure Signed-off-by: Krishna Kondaka <[email protected]> --------- Signed-off-by: Krishna Kondaka <[email protected]> Co-authored-by: Krishna Kondaka <[email protected]> (cherry picked from commit 515cf61)

) * Fix race condition in SqsWorker when acknowledgements are enabled Signed-off-by: Krishna Kondaka <[email protected]> * Modified to do the synchronization in the acknowledgement set framework Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Removed unused variable Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comment and fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Fixed checkStyle failure Signed-off-by: Krishna Kondaka <[email protected]> --------- Signed-off-by: Krishna Kondaka <[email protected]> Co-authored-by: Krishna Kondaka <[email protected]> Signed-off-by: George Chen <[email protected]>

…ensearch-project#3001) * Fix race condition in SqsWorker when acknowledgements are enabled Signed-off-by: Krishna Kondaka <[email protected]> * Modified to do the synchronization in the acknowledgement set framework Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Removed unused variable Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comment and fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Fixed failing tests Signed-off-by: Krishna Kondaka <[email protected]> * Fixed checkStyle failure Signed-off-by: Krishna Kondaka <[email protected]> --------- Signed-off-by: Krishna Kondaka <[email protected]> Co-authored-by: Krishna Kondaka <[email protected]> Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>

Fix race condition in SqsWorker when acknowledgements are enabled

b2f6a8a

Signed-off-by: Krishna Kondaka <[email protected]>

kkondaka requested review from chenqi0805, engechas, graytaylor0, dinujoh, cmanning09, asifsmohammed, dlvenable and oeyh as code owners July 11, 2023 07:29

Krishna Kondaka added 2 commits July 11, 2023 15:00

Modified to do the synchronization in the acknowledgement set framework

5fac36b

Signed-off-by: Krishna Kondaka <[email protected]>

Fixed failing tests

936f32d

Signed-off-by: Krishna Kondaka <[email protected]>

engechas reviewed Jul 11, 2023

View reviewed changes

Removed unused variable

51a1227

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable requested changes Jul 11, 2023

View reviewed changes

Krishna Kondaka added 4 commits July 11, 2023 18:37

Addressed review comment and fixed failing tests

2935c26

Signed-off-by: Krishna Kondaka <[email protected]>

Addressed review comments

983e778

Signed-off-by: Krishna Kondaka <[email protected]>

Fixed failing tests

29cebdd

Signed-off-by: Krishna Kondaka <[email protected]>

Fixed checkStyle failure

46c5bfc

Signed-off-by: Krishna Kondaka <[email protected]>

engechas approved these changes Jul 11, 2023

View reviewed changes

dlvenable approved these changes Jul 11, 2023

View reviewed changes

kkondaka merged commit 515cf61 into opensearch-project:main Jul 11, 2023
24 checks passed

kkondaka added the backport 2.3 label Jul 11, 2023

kkondaka deleted the s3-race-cond branch May 13, 2024 05:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition in SqsWorker when acknowledgements are enabled #3001

Fix race condition in SqsWorker when acknowledgements are enabled #3001

kkondaka commented Jul 11, 2023

engechas left a comment

engechas Jul 11, 2023

dlvenable Jul 11, 2023

kkondaka Jul 11, 2023

dlvenable Jul 11, 2023

opensearch-trigger-bot bot commented Jul 11, 2023

Fix race condition in SqsWorker when acknowledgements are enabled #3001

Fix race condition in SqsWorker when acknowledgements are enabled #3001

Conversation

kkondaka commented Jul 11, 2023

Description

Issues Resolved

Check List

engechas left a comment

Choose a reason for hiding this comment

engechas Jul 11, 2023

Choose a reason for hiding this comment

dlvenable Jul 11, 2023

Choose a reason for hiding this comment

kkondaka Jul 11, 2023

Choose a reason for hiding this comment

dlvenable Jul 11, 2023

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jul 11, 2023