Skip to content
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

Sqs Source initial changes #2786

Merged
merged 30 commits into from
Jun 26, 2023

Conversation

udaych20
Copy link
Contributor

@udaych20 udaych20 commented May 31, 2023

Description

Sqs Source plugin implementation including junit test cases for #2679

Issues Resolved

Resolves the #2679

Check List

  • New functionality includes testing.
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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: Uday Kumar Chintala <udaych20@gmail.com>
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #2786 (05ba0b8) into main (e30e818) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 05ba0b8 differs from pull request most recent head c5206a4. Consider uploading reports for the commit c5206a4 to get more accurate results

@@             Coverage Diff              @@
##               main    #2786      +/-   ##
============================================
- Coverage     94.15%   94.14%   -0.02%     
+ Complexity     2442     2440       -2     
============================================
  Files           278      278              
  Lines          6759     6744      -15     
  Branches        551      551              
============================================
- Hits           6364     6349      -15     
  Misses          254      254              
  Partials        141      141              

see 1 file with indirect coverage changes

@udaych20 udaych20 requested a review from dlvenable June 5, 2023 10:07
Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

Thanks! This is looking a lot better.

@udaych20 udaych20 requested a review from dlvenable June 8, 2023 12:09
Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
Signed-off-by: Uday Chintala <udaych20@gmail.com>
kkondaka
kkondaka previously approved these changes Jun 12, 2023
@kkondaka
Copy link
Collaborator

One test is failing.

SqsSourceConfigTest > sqs_source_configuration_test() FAILED
    java.lang.AssertionError at SqsSourceConfigTest.java:45

Please fix it.

Signed-off-by: Uday Chintala <udaych20@gmail.com>
Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
@udaych20 udaych20 requested a review from kkondaka June 13, 2023 07:58
@udaych20
Copy link
Contributor Author

One test is failing.

SqsSourceConfigTest > sqs_source_configuration_test() FAILED
    java.lang.AssertionError at SqsSourceConfigTest.java:45

Please fix it.

Done.

});
try {
if(!events.isEmpty())
buffer.writeAll(events, bufferTimeOut);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@graytaylor0 do you if this is the best approach here to write to buffer with a small set of events (max of 10)? Or we could use similar concept as S3 source where we accumulate records in BufferAccumulator and flush configurable batches.

Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
asifsmohammed
asifsmohammed previously approved these changes Jun 22, 2023
Comment on lines +38 to +40
static final Duration BUFFER_TIMEOUT = Duration.ofSeconds(10);

static final int NO_OF_RECORDS_TO_ACCUMULATE = 100;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, It will be available in the incremental PR.

@asifsmohammed asifsmohammed dismissed dlvenable’s stale review June 26, 2023 18:11

All the feedback by David is addressed

@asifsmohammed asifsmohammed merged commit 497c74d into opensearch-project:main Jun 26, 2023
MaGonzalMayedo pushed a commit to MaGonzalMayedo/data-prepper that referenced this pull request Jul 25, 2023
* Sqs Source implementation

Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>

---------

Signed-off-by: Uday Kumar Chintala <udaych20@gmail.com>
Signed-off-by: Uday Chintala <udaych20@gmail.com>
Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>
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.

5 participants