Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Global configuration for deletion policy #621

Closed
wants to merge 5 commits into from
Closed

Global configuration for deletion policy #621

wants to merge 5 commits into from

Conversation

MatejNedic
Copy link
Contributor

AWS SQS: Add global configuration for deletionPolicy #188

@MatejNedic MatejNedic marked this pull request as draft June 30, 2020 10:53
@MatejNedic MatejNedic marked this pull request as ready for review June 30, 2020 10:53
maciejwalkowiak
maciejwalkowiak suggested changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

Thanks @MatejNedic for another contribution! I am not sure if this change lasts till 3.0 as I hope for solid SQS integration redesign, but I think it can go with 2.3.

...src/main/java/org/springframework/cloud/aws/messaging/listener/SqsMessageDeletionPolicy.java Outdated
* If this is set in SqsListener, it will use global value set for specific QueueMessageHandlerFactory.
* Default, if not changed is set to NO_REDRIVE.
*/
GLOBAL
Copy link
Contributor

Choose a reason for hiding this comment

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

I think DEFAULT would be a better name. global -> default should be applied then in every other changed file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True default is better. Done

...ging/src/main/java/org/springframework/cloud/aws/messaging/listener/QueueMessageHandler.java Outdated
@@ -73,6 +74,7 @@
static final String LOGICAL_RESOURCE_ID = "LogicalResourceId";
static final String ACKNOWLEDGMENT = "Acknowledgment";
static final String VISIBILITY = "Visibility";
private SqsMessageDeletionPolicy globalSqsMessageDeletionPolicy = SqsMessageDeletionPolicy.NO_REDRIVE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this field as final and pass value in constructor? Regarding name defaultMessageDeletionPolicy would fit better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@maciejwalkowiak maciejwalkowiak added component: sqs SQS integration related issue status: waiting-for-feedback We need additional information before we can continue type: enhancement A general enhancement and removed waiting-for-triage labels Jul 6, 2020
@maciejwalkowiak maciejwalkowiak added this to the 2.3 milestone Jul 6, 2020
maciejwalkowiak
maciejwalkowiak approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

I will do some minor polishing but other than that looks good!

@maciejwalkowiak maciejwalkowiak removed the status: waiting-for-feedback We need additional information before we can continue label Jul 7, 2020
eddumelendez
eddumelendez suggested changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

Thanks @MatejNedic for another great contribution. I have left just small comments.

...ging/src/main/java/org/springframework/cloud/aws/messaging/listener/QueueMessageHandler.java Outdated
@@ -73,15 +74,18 @@
static final String LOGICAL_RESOURCE_ID = "LogicalResourceId";
static final String ACKNOWLEDGMENT = "Acknowledgment";
static final String VISIBILITY = "Visibility";
private final SqsMessageDeletionPolicy defaultSqsMessageDeletionPolicy;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra space. Also, can be renamed to sqsMessageDeletionPolicy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...est/java/org/springframework/cloud/aws/messaging/config/annotation/SqsConfigurationTest.java Outdated
@@ -119,6 +121,9 @@ void messageHandler_withFactoryConfiguration_shouldUseCustomValues()
assertThat(messageHandler.getCustomReturnValueHandlers().get(0)).isEqualTo(
ConfigurationWithCustomizedMessageHandler.CUSTOM_RETURN_VALUE_HANDLER);

Object sqsMessageDeletionPolicy = ReflectionTestUtils.getField(messageHandler,"defaultSqsMessageDeletionPolicy");
Copy link
Contributor

Choose a reason for hiding this comment

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

there should be a space before "defaultSqsMessageDeletionPolicy"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...est/java/org/springframework/cloud/aws/messaging/config/annotation/SqsConfigurationTest.java Outdated
ConfigurationWithCustomizedMessageHandlerGlobalDeletionPolicy.CUSTOM_RETURN_VALUE_HANDLER);


Object sqsMessageDeletionPolicy = ReflectionTestUtils.getField(messageHandler,"defaultSqsMessageDeletionPolicy");
Copy link
Contributor

Choose a reason for hiding this comment

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

there should be a space before "defaultSqsMessageDeletionPolicy"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...src/main/java/org/springframework/cloud/aws/messaging/config/QueueMessageHandlerFactory.java Outdated
@@ -52,6 +54,8 @@

private ResourceIdResolver resourceIdResolver;

private SqsMessageDeletionPolicy defaultSqsMessageDeletionPolicy = SqsMessageDeletionPolicy.NO_REDRIVE;
Copy link
Contributor

Choose a reason for hiding this comment

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

just sqsMessageDeletionPolicy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@maciejwalkowiak
Copy link
Contributor

Thanks! I am merging it, don't worry about conflicts.

maciejwalkowiak pushed a commit to maciejwalkowiak/spring-cloud-aws that referenced this pull request Oct 15, 2020
@MatejNedic @maciejwalkowiak
Global deletion policy for sqs listeners.
40692c0 
Fixes spring-atticgh-188
Closes spring-atticgh-621
maciejwalkowiak pushed a commit to maciejwalkowiak/spring-cloud-aws that referenced this pull request Oct 15, 2020
@MatejNedic @maciejwalkowiak
Global deletion policy for sqs listeners.
19b8796 
Fixes spring-attic#188
Closes spring-attic#621
juho9000 pushed a commit to juho9000/spring-cloud-aws that referenced this pull request Apr 29, 2021
@MatejNedic @maciejwalkowiak
Global deletion policy for sqs listeners.
d13b5e0 
Fixes spring-attic/spring-cloud-aws#188
Closes spring-attic/spring-cloud-aws#621
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eddumelendez eddumelendez eddumelendez requested changes

@maciejwalkowiak maciejwalkowiak maciejwalkowiak approved these changes

Assignees
No one assigned
Labels
component: sqs SQS integration related issue type: enhancement A general enhancement
Milestone
2.3
Development

Successfully merging this pull request may close these issues.
4 participants
@MatejNedic @maciejwalkowiak @eddumelendez @spring-projects-issues