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.

Sign up for GitHub

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

Jump to bottom

Sns Sink Plugin with junit test cases #2995

Merged
merged 28 commits into from
Aug 4, 2023
Merged

Sns Sink Plugin with junit test cases #2995

merged 28 commits into from
Aug 4, 2023

Conversation

udaych20
Copy link
Contributor

Description

Draft PR for sns sink #2938

Issues Resolved

This PR consists sns-sink plugin code with junit test cases.

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.

@udaych20 udaych20 marked this pull request as ready for review July 12, 2023 05:32
graytaylor0
graytaylor0 reviewed Jul 25, 2023
View reviewed changes
...er-plugins/sns-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/SNSSinkConfig.java Outdated Show resolved Hide resolved
...r-plugins/sns-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/SNSSinkService.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/dlq/DlqPushHandler.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/dlq/DlqPushHandler.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceTest.java Outdated Show resolved Hide resolved
data-prepper-plugins/sns-sink/build.gradle Outdated Show resolved Hide resolved
dlvenable
dlvenable requested changes Jul 28, 2023
View reviewed changes
...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceIT.java Outdated Show resolved Hide resolved
...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceIT.java Outdated Show resolved Hide resolved
...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceIT.java Outdated Show resolved Hide resolved
...-sink/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceIT.java Outdated Show resolved Hide resolved
...er-plugins/sns-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/SNSSinkConfig.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceTest.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceTest.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceTest.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceTest.java Outdated Show resolved Hide resolved
...ugins/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/SNSSinkServiceTest.java Outdated Show resolved Hide resolved
dlvenable
dlvenable requested changes Aug 1, 2023
View reviewed changes
...k/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/sns/SnsSinkServiceIT.java Outdated

private List<String> readMessagesFromSNSTopicQueue(List<String> inputRecords, final String sqsQueue) {
final List<Message> messages = new ArrayList<>();
do{
Copy link
Member

Choose a reason for hiding this comment

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

This should use Awaitility to wait for a maximum period of time. Perhaps 1 minute.

As it is, this can get stuck indefinitely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dlvenable for your feedback, we have incorporated the changes and available for review.

...lugins/sns-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/sns/SnsSinkConfig.java Outdated
private String topicArn;

@JsonProperty("message_group_id")
private String messageGroupId;
public String messageGroupId;
Copy link
Member

Choose a reason for hiding this comment

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

This should be private and provide a public getter - getMessageGroupId().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dlvenable for your feedback, we have incorporated the changes and available for review.

...s/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/sns/SnsSinkServiceTest.java
Collection<Record<Event>> records = List.of(eventRecord);
snsSinkService.output(records);
verify(numberOfRecordsSuccessCounter).increment(records.size());
}
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned this in an earlier comment.
#2995 (comment)

We need to verify a couple of things:

  1. The sink calls the client.
  2. The data that the sink provides to the client is correct.

It should look somewhat like the following.

final PublishBatchRequest request = createPublicRequestByTopic(topicName, processRecordsList);
final PublishBatchResponse publishBatchResponse = snsClient.publishBatch(request);

final ArgumentCaptor<PublishBatchRequest> publishBatchRequestCaptor = ArgumentCaptor.forClass(PublishBatchRequest.class);
verify(snsClient).publishBash(publishBatchRequestCaptor.capture());

final PublishBatchRequest actualRequest = publishBatchRequestCaptor.getValue();

assertThat(actualRequest.topicArn(), equalTo(topicArn));
assertThat(actualRequest.publishBatchRequestEntries().size(), equalTo(1));
// Dive into the first publish batch request entry and verify that it has the correct string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dlvenable for your feedback, we have incorporated the changes and available for review.

...s/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/sns/SnsSinkServiceTest.java Outdated Show resolved Hide resolved
...s/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/sns/SnsSinkServiceTest.java Show resolved Hide resolved
...s/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/sns/SnsSinkServiceTest.java Show resolved Hide resolved
...s/sns-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/sns/SnsSinkServiceTest.java Show resolved Hide resolved
graytaylor0
graytaylor0 previously approved these changes Aug 3, 2023
View reviewed changes
dlvenable
dlvenable previously approved these changes Aug 4, 2023
View reviewed changes
@dlvenable
Copy link
Member

@udaych20 , Please rebase your branch from main and force push. Another commit has created the settings.gradle file so we cannot automatically merge.

@udaych20 udaych20 dismissed stale reviews from dlvenable and graytaylor0 via 7088650 August 4, 2023 16:32
@udaych20
Copy link
Contributor Author

udaych20 commented Aug 4, 2023

@udaych20 , Please rebase your branch from main and force push. Another commit has created the settings.gradle file so we cannot automatically merge.

@dlvenable done.

@dlvenable
Copy link
Member

This PR was previously approved by @graytaylor0 , and has only been rebased. So I'll go ahead and merge.

@dlvenable dlvenable merged commit bdbb174 into opensearch-project:main Aug 4, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlvenable dlvenable dlvenable approved these changes

@graytaylor0 graytaylor0 graytaylor0 left review comments

@chenqi0805 chenqi0805 Awaiting requested review from chenqi0805 chenqi0805 is a code owner

@engechas engechas Awaiting requested review from engechas engechas is a code owner

@dinujoh dinujoh Awaiting requested review from dinujoh dinujoh is a code owner

@kkondaka kkondaka Awaiting requested review from kkondaka kkondaka is a code owner

@cmanning09 cmanning09 Awaiting requested review from cmanning09

@oeyh oeyh Awaiting requested review from oeyh oeyh is a code owner

@asifsmohammed asifsmohammed Awaiting requested review from asifsmohammed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@udaych20 @dlvenable @asifsmohammed @graytaylor0