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

Add config to enable PITR #1365

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Conversation

lucienlu-aws
Copy link
Contributor

Issue #, if available:

Description of changes:

  • Adds a configurable to enable PITR on the lease table leaseTablePitrEnabled
  • Adds configurable in multilang for leaseTableDeletionProtectionEnabled and leaseTablePitrEnabled

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

private static final boolean PITR_ENABLED = true;
private static final Collection<Tag> EMPTY_TAGS = DefaultSdkAutoConstructList.getInstance();
private static final Collection<Tag> TAGS =
Collections.singletonList(Tag.builder().key("foo").value("bar").build());
Copy link

Choose a reason for hiding this comment

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

Thank you for making this more consistent.

@@ -159,7 +162,7 @@ public DynamoDBLeaseRefresher(
tableCreatorCallback,
dynamoDbRequestTimeout,
BillingMode.PAY_PER_REQUEST,
false);
LeaseManagementConfig.DEFAULT_LEASE_TABLE_DELETION_PROTECTION_ENABLED);
Copy link

Choose a reason for hiding this comment

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

Thank you for this.

Copy link

@dcpavel dcpavel Jul 12, 2024

Choose a reason for hiding this comment

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

We check that leaseTableDeletionProtectionEnabled() and leaseTablePitrEnabled() equal true, but this will give a false positive if the default changes to true and setLeaseTable...Enabled() methods break.

This is a bit of a nit since it appears this methodology was repeated with other tests. Just including it for consideration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've added two more tests to set the false value as well

Copy link

@dcpavel dcpavel left a comment

Choose a reason for hiding this comment

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

This looks okay. I particularly appreciate the inclusion of some new constants, and I included a note about potential false positives in the tests, but I don't think it should prevent merging.

MultiLangDaemonConfiguration.ResolvedConfiguration resolvedConfiguration =
configuration.resolvedConfiguration(shardRecordProcessorFactory);

assertThat(resolvedConfiguration.leaseManagementConfig.leaseTableDeletionProtectionEnabled(), equalTo(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit, but this could just be assertTrue(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 308 to 310
.pointInTimeRecoverySpecification(PointInTimeRecoverySpecification.builder()
.pointInTimeRecoveryEnabled(true)
.build())
Copy link
Contributor

Choose a reason for hiding this comment

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

https://sdk.amazonaws.com/java/api/2.0.0/software/amazon/awssdk/services/dynamodb/model/UpdateContinuousBackupsRequest.Builder.html#pointInTimeRecoverySpecification-java.util.function.Consumer-

Instead of passing in a PointInTimeRecoverySpecification, you can pass in a Consumer<PointInTimeRecoverySpecification.Builder> like so:

Suggested change
.pointInTimeRecoverySpecification(PointInTimeRecoverySpecification.builder()
.pointInTimeRecoveryEnabled(true)
.build())
.pointInTimeRecoverySpecification(builder->builder.pointInTimeRecoveryEnabled(true))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this shorthand, did not know about it. Updated

@lucienlu-aws lucienlu-aws merged commit 1c0c41c into awslabs:master Jul 15, 2024
2 checks passed
@lucienlu-aws lucienlu-aws deleted the pitr-config branch July 15, 2024 21:15
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.

3 participants