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

Change the default for the DLQ to be off by default #51

Merged
merged 2 commits into from
Feb 14, 2024
Merged

Change the default for the DLQ to be off by default #51

merged 2 commits into from
Feb 14, 2024

Conversation

gklijs
Copy link

@gklijs gklijs commented Feb 13, 2024

Fixes #50

@gklijs gklijs added Priority 1: Must Type: Enhancement Backend This issue contains backend changes labels Feb 13, 2024
@gklijs gklijs requested a review from a team February 13, 2024 10:43
@gklijs gklijs self-assigned this Feb 13, 2024
@gklijs gklijs requested review from corradom, CodeDrivenMitch, smcvb, sandjelkovic and stefanmirkovic and removed request for a team February 13, 2024 10:43
@abuijze
Copy link
Contributor

abuijze commented Feb 13, 2024

Just wondering, since the concern is that potentially business-specific data is transferred over the wire without explicit permission of the app developers, isn't it sufficient to default to masked?

@gklijs
Copy link
Author

gklijs commented Feb 13, 2024

It might be fine to switch to masked as the default. That's not how I interpreted the story through. Maybe @SaraTorrey can decide. I don't know what the worries are. For example, with masked, anyone could still initiate a retry, which might not be something we allow by default.

@gklijs gklijs force-pushed the feature/turn-dlq-off-by-default branch 2 times, most recently from a238272 to 3fd831e Compare February 13, 2024 13:16
@SaraTorrey
Copy link

SaraTorrey commented Feb 13, 2024 •
edited
Loading

After discussing various options with Gerard, I think setting the default to off is a better option for security and privacy reasons. We just have to be sure that the information and options are clearly defined in our documentation.

smcvb
smcvb approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

LGTM!
Another question though: wdyt about expanding the setup instructions to clarify the DLQ option is off by default?

@gklijs gklijs force-pushed the feature/turn-dlq-off-by-default branch from 3fd831e to 37e06e7 Compare February 14, 2024 11:02
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Feb 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@gklijs gklijs requested a review from smcvb February 14, 2024 11:06
@gklijs
Copy link
Author

gklijs commented Feb 14, 2024

I noticed discrepancies in the readme, so I made some adjustments. Hence, asking for a review again.

smcvb
smcvb approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Still think this looks fine.

@gklijs gklijs merged commit d075e95 into main Feb 14, 2024
6 checks passed
@gklijs gklijs deleted the feature/turn-dlq-off-by-default branch February 14, 2024 13:00
@gklijs gklijs added this to the V1.4.0 milestone Feb 15, 2024
@smcvb smcvb changed the title Change the default for the dlq to be off by default. Change the default for the DLQ to be off by default Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smcvb smcvb smcvb approved these changes

@corradom corradom Awaiting requested review from corradom corradom was automatically assigned from AxonIQ/axoniq-console

@CodeDrivenMitch CodeDrivenMitch Awaiting requested review from CodeDrivenMitch CodeDrivenMitch was automatically assigned from AxonIQ/axoniq-console

@sandjelkovic sandjelkovic Awaiting requested review from sandjelkovic sandjelkovic was automatically assigned from AxonIQ/axoniq-console

@stefanmirkovic stefanmirkovic Awaiting requested review from stefanmirkovic stefanmirkovic was automatically assigned from AxonIQ/axoniq-console

Assignees

@gklijs gklijs

Labels
Backend This issue contains backend changes Priority 1: Must Type: Enhancement
Projects
None yet
Milestone
V1.4.0
Development

Successfully merging this pull request may close these issues.

Switch the Dead-Letter Queue support off by default
4 participants
@gklijs @abuijze @SaraTorrey @smcvb