From 92bce127e93c63c0960aa61138398eee5220104b Mon Sep 17 00:00:00 2001 From: William Lo Date: Fri, 20 Dec 2024 16:18:22 -0500 Subject: [PATCH] Add validators for History / Snapshot Retention Policy (#259) ## Summary Supports defining a policy to control the versions of snapshots tables to retain. The policy will support 2 types of configurations: 1. Time-based (e.g. 3 days) 2. Count-based (e.g. 10 versions) It can also support a combination of the 2 policies, in which case it will retain versions that fall under both categories (e.g. only keep versions newer than 3 days AND within 10 versions). This PR also refactors the PolicySpecValidator to accurately depict that it's referencing to partition-based retention of tables. In the future, we should refactor these policy classes to follow some `Policy` interface for better standardization for future policy support (duplicated interfaces between Replication, Partition Retention, Retain Snapshot). This PR also defines the maximums for snapshot retention defaults (TODO: make this configurable/static?) **The current maximums defined is 3 days and 100 versions** ## Changes - [x] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done - [x] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] Added new tests for the changes made. - [x] Updated existing tests to reflect the changes made. - [ ] No tests added or updated. Please explain why. If unsure, please feel free to ask for help. - [ ] Some other form of testing like staging or soak time in production. Please explain. Ran on local docker and tested using postman by creating a new table with the following: ``` { "tableId": "t2", "databaseId": "d3", "baseTableVersion": "INITIAL_VERSION", "clusterId": "LocalFSCluster", "schema": "{\"type\": \"struct\", \"fields\": [{\"id\": 1,\"required\": true,\"name\": \"id\",\"type\": \"string\"},{\"id\": 2,\"required\": true,\"name\": \"name\",\"type\": \"string\"},{\"id\": 3,\"required\": true,\"name\": \"ts\",\"type\": \"timestamp\"}]}", "tableProperties": { "key": "value" }, "policies": { "sharingEnabled": "true", "history": {"maxAge": 1, "granularity": "DAY", "versions": 2} } } ``` Tested getTable after and confirmed the policy was showing correctly. Also tested the negative cases (high version count, negative numbers) which worked as well. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request. --- .../OpenHouseTableOperationsTest.java | 59 ++++++++ .../javaclient/OpenHouseTableOperations.java | 6 + .../spec/v0/request/components/History.java | 40 ++++++ .../spec/v0/request/components/Policies.java | 7 + .../impl/HistoryPolicySpecValidator.java | 97 +++++++++++++ .../impl/OpenHouseTablesApiValidator.java | 21 ++- ...java => RetentionPolicySpecValidator.java} | 6 +- .../impl/HistoryPolicySpecValidatorTest.java | 131 ++++++++++++++++++ ... => RetentionPolicySpecValidatorTest.java} | 10 +- .../tables/e2e/h2/TablesControllerTest.java | 71 ++++++++++ .../tables/mock/api/TablesValidatorTest.java | 48 +++++++ .../mock/mapper/PoliciesSpecMapperTest.java | 9 ++ .../tables/model/TableModelConstants.java | 15 +- 13 files changed, 507 insertions(+), 13 deletions(-) create mode 100644 services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/History.java create mode 100644 services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidator.java rename services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/{PoliciesSpecValidator.java => RetentionPolicySpecValidator.java} (96%) create mode 100644 services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidatorTest.java rename services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/{PoliciesSpecValidatorTest.java => RetentionPolicySpecValidatorTest.java} (97%) diff --git a/integrations/java/iceberg-1.2/openhouse-java-itest/src/test/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperationsTest.java b/integrations/java/iceberg-1.2/openhouse-java-itest/src/test/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperationsTest.java index 60277340d..b17726c14 100644 --- a/integrations/java/iceberg-1.2/openhouse-java-itest/src/test/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperationsTest.java +++ b/integrations/java/iceberg-1.2/openhouse-java-itest/src/test/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperationsTest.java @@ -5,6 +5,7 @@ import com.linkedin.openhouse.gen.tables.client.api.SnapshotApi; import com.linkedin.openhouse.gen.tables.client.api.TableApi; import com.linkedin.openhouse.gen.tables.client.model.CreateUpdateTableRequestBody; +import com.linkedin.openhouse.gen.tables.client.model.History; import com.linkedin.openhouse.gen.tables.client.model.Policies; import com.linkedin.openhouse.gen.tables.client.model.PolicyTag; import com.linkedin.openhouse.gen.tables.client.model.Retention; @@ -344,4 +345,62 @@ public void testPoliciesReplicationExistsUpdateExistsForMultiple() { updatedPolicies.getReplication().getConfig().get(1).getInterval(), "2D"); Assertions.assertEquals(updatedPolicies.getReplication().getConfig().size(), 2); } + + @Test + public void testPoliciesHistoryInMetadataNoUpdate() { + Map props = new HashMap<>(); + props.put( + "policies", + "{\"history\": {\"maxAge\": \"1\", \"granularity\": \"DAY\", \"versions\": \"2\"}}"); + TableMetadata metadata = mock(TableMetadata.class); + when(metadata.properties()).thenReturn(props); + OpenHouseTableOperations openHouseTableOperations = mock(OpenHouseTableOperations.class); + when(openHouseTableOperations.buildUpdatedPolicies(metadata)).thenCallRealMethod(); + Policies updatedPolicies = openHouseTableOperations.buildUpdatedPolicies(metadata); + Assertions.assertNotNull(updatedPolicies); + Assertions.assertEquals(1, updatedPolicies.getHistory().getMaxAge()); + Assertions.assertEquals( + History.GranularityEnum.DAY, updatedPolicies.getHistory().getGranularity()); + Assertions.assertEquals(2, updatedPolicies.getHistory().getVersions()); + } + + @Test + public void testNoPoliciesHistoryExistsButUpdateExists() { + Map props = new HashMap<>(); + props.put( + "updated.openhouse.policy", + "{\"history\": {\"maxAge\": \"1\", \"granularity\": \"DAY\", \"versions\": \"2\"}}"); + TableMetadata metadata = mock(TableMetadata.class); + when(metadata.properties()).thenReturn(props); + OpenHouseTableOperations openHouseTableOperations = mock(OpenHouseTableOperations.class); + when(openHouseTableOperations.buildUpdatedPolicies(metadata)).thenCallRealMethod(); + Policies updatedPolicies = openHouseTableOperations.buildUpdatedPolicies(metadata); + Assertions.assertNotNull(updatedPolicies); + Assertions.assertEquals(1, updatedPolicies.getHistory().getMaxAge()); + Assertions.assertEquals( + History.GranularityEnum.DAY, updatedPolicies.getHistory().getGranularity()); + Assertions.assertEquals(2, updatedPolicies.getHistory().getVersions()); + } + + @Test + public void testPoliciesHistoryExistsUpdate() { + Map props = new HashMap<>(); + props.put( + "openhouse.policy", + "{\"history\": {\"maxAge\": \"2\", \"granularity\": \"HOUR\", \"versions\": \"3\"}}"); + props.put( + "updated.openhouse.policy", + "{\"history\": {\"maxAge\": \"1\", \"granularity\": \"DAY\", \"versions\": \"2\"}, \"sharingEnabled\": true}"); + TableMetadata metadata = mock(TableMetadata.class); + when(metadata.properties()).thenReturn(props); + OpenHouseTableOperations openHouseTableOperations = mock(OpenHouseTableOperations.class); + when(openHouseTableOperations.buildUpdatedPolicies(metadata)).thenCallRealMethod(); + Policies updatedPolicies = openHouseTableOperations.buildUpdatedPolicies(metadata); + Assertions.assertNotNull(updatedPolicies); + Assertions.assertEquals(1, updatedPolicies.getHistory().getMaxAge()); + Assertions.assertEquals( + History.GranularityEnum.DAY, updatedPolicies.getHistory().getGranularity()); + Assertions.assertEquals(2, updatedPolicies.getHistory().getVersions()); + Assertions.assertEquals(true, updatedPolicies.getSharingEnabled()); + } } diff --git a/integrations/java/iceberg-1.2/openhouse-java-runtime/src/main/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperations.java b/integrations/java/iceberg-1.2/openhouse-java-runtime/src/main/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperations.java index 83db9a0f0..374903558 100644 --- a/integrations/java/iceberg-1.2/openhouse-java-runtime/src/main/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperations.java +++ b/integrations/java/iceberg-1.2/openhouse-java-runtime/src/main/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperations.java @@ -194,6 +194,7 @@ Policies buildUpdatedPolicies(TableMetadata metadata) { if (patchUpdatedPolicy.getRetention() != null) { policies.setRetention(patchUpdatedPolicy.getRetention()); } + // Update sharing config if (patchUpdatedPolicy.getSharingEnabled() != null) { policies.sharingEnabled(patchUpdatedPolicy.getSharingEnabled()); @@ -215,6 +216,11 @@ Policies buildUpdatedPolicies(TableMetadata metadata) { if (patchUpdatedPolicy.getReplication() != null) { policies.replication(patchUpdatedPolicy.getReplication()); } + // Update history config + if (patchUpdatedPolicy.getHistory() != null) { + policies.setHistory(patchUpdatedPolicy.getHistory()); + } + return policies; } diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/History.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/History.java new file mode 100644 index 000000000..42492409c --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/History.java @@ -0,0 +1,40 @@ +package com.linkedin.openhouse.tables.api.spec.v0.request.components; + +import io.swagger.v3.oas.annotations.media.Schema; +import javax.validation.Valid; +import javax.validation.constraints.PositiveOrZero; +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NoArgsConstructor; + +@Builder(toBuilder = true) +@EqualsAndHashCode +@Getter +@AllArgsConstructor(access = AccessLevel.PROTECTED) +@NoArgsConstructor(access = AccessLevel.PROTECTED) +public class History { + @Schema( + description = "Time period in count to keep the snapshot history on the table", + example = "3,4,5") + @PositiveOrZero( + message = + "Incorrect count specified. history.maxAge has to be a positive integer or zero if undefined") + @Valid + int maxAge; + + @Schema(description = "time period granularity for the snapshot history", example = "hour, day") + @Valid + TimePartitionSpec.Granularity granularity; + + @Schema( + description = + "Number of snapshots to keep within history for the table after snapshot expiration", + example = "3,4,5") + @PositiveOrZero( + message = + "Incorrect count specified. history.versions has to be a positive integer or zero if undefined") + int versions; +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/Policies.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/Policies.java index c855bc349..2a40a6856 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/Policies.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/Policies.java @@ -48,4 +48,11 @@ public class Policies { example = "{replication:{config:[{destination: clusterA, interval: 12H}]}}") @Valid Replication replication; + + @Schema( + description = + "History as required in /tables API request. This field holds the snapshot retention specification.", + example = "{history:{maxAge:3, granularity: 'day', versions: 5}}") + @Valid + History history; } diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidator.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidator.java new file mode 100644 index 000000000..9e3d45c0f --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidator.java @@ -0,0 +1,97 @@ +package com.linkedin.openhouse.tables.api.validator.impl; + +import com.linkedin.openhouse.common.api.spec.TableUri; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.History; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.TimePartitionSpec; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Component; + +@Slf4j +@Component +public class HistoryPolicySpecValidator { + + private String failureMessage = ""; + private String errorField = ""; + + protected boolean validate(History history, TableUri tableUri) { + if (history != null) { + if (history.getMaxAge() <= 0 && history.getVersions() <= 0) { + failureMessage = + String.format( + "Must define either a time based retention or count based retention for snapshots in table %s", + tableUri); + return false; + } + + if (history.getGranularity() == null && history.getMaxAge() > 0 + || history.getGranularity() != null && history.getMaxAge() <= 0) { + failureMessage = + String.format( + "Incorrect maxAge specified. history.maxAge must be defined together with history.granularity for table %s", + tableUri); + return false; + } + + if (!validateHistoryConfigMaxAgeWithinBounds(history)) { + failureMessage = + String.format( + "History for the table [%s] max age must be between 1 to 3 days", tableUri); + return false; + } + + if (!validateHistoryConfigVersionsWithinBounds(history)) { + failureMessage = + String.format("History for the table [%s] must be between 2 to 100 versions", tableUri); + return false; + } + } + return true; + } + + /** + * Validate that the amount of time to retain history of table snapshots is between 1 and 3 days + * + * @param history + * @return + */ + protected boolean validateHistoryConfigMaxAgeWithinBounds(History history) { + int maxAge = history.getMaxAge(); + TimePartitionSpec.Granularity granularity = history.getGranularity(); + // if maxAge is 0 then consider it undefined and refer to default for snapshot expiration + if (maxAge == 0) { + return true; + } + + if (granularity.equals(TimePartitionSpec.Granularity.HOUR) + || granularity.equals(TimePartitionSpec.Granularity.DAY)) { + return (maxAge <= 3 && granularity.equals(TimePartitionSpec.Granularity.DAY) + || maxAge <= 72 && granularity.equals(TimePartitionSpec.Granularity.HOUR)) + && (maxAge >= 1 && granularity.equals(TimePartitionSpec.Granularity.DAY) + || maxAge >= 24 && granularity.equals(TimePartitionSpec.Granularity.HOUR)); + } + + return false; + } + + /* + * Validate that the number of versions to retain history of table snapshots is between 2 and 100 + * We want at least 2 versions so that users can always rollback to at least 1 version before a commit + */ + protected boolean validateHistoryConfigVersionsWithinBounds(History history) { + if (history.getVersions() + == 0) { // versions is 0 then consider it undefined and refer to default for snapshot + // expiration + return true; + } + int versions = history.getVersions(); + return versions >= 2 && versions <= 100; + } + + public String getMessage() { + return failureMessage; + } + + public String getField() { + return errorField; + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java index c2744f7ee..5a81df41b 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java @@ -27,12 +27,14 @@ public class OpenHouseTablesApiValidator implements TablesApiValidator { @Autowired private Validator validator; - @Autowired private PoliciesSpecValidator policiesSpecValidator; + @Autowired private RetentionPolicySpecValidator retentionPolicySpecValidator; @Autowired private ClusteringSpecValidator clusteringSpecValidator; @Autowired private ReplicationConfigValidator replicationConfigValidator; + @Autowired private HistoryPolicySpecValidator historyPolicySpecValidator; + @Override public void validateGetTable(String databaseId, String tableId) { List validationFailures = new ArrayList<>(); @@ -132,7 +134,7 @@ private void validatePolicies(CreateUpdateTableRequestBody createUpdateTableRequ .clusterId(createUpdateTableRequestBody.getClusterId()) .databaseId(createUpdateTableRequestBody.getDatabaseId()) .build(); - if (!policiesSpecValidator.validate( + if (!retentionPolicySpecValidator.validate( createUpdateTableRequestBody.getPolicies(), createUpdateTableRequestBody.getTimePartitioning(), tableUri, @@ -141,7 +143,8 @@ private void validatePolicies(CreateUpdateTableRequestBody createUpdateTableRequ Arrays.asList( String.format( "%s : %s", - policiesSpecValidator.getField(), policiesSpecValidator.getMessage()))); + retentionPolicySpecValidator.getField(), + retentionPolicySpecValidator.getMessage()))); } if (createUpdateTableRequestBody.getPolicies() != null && createUpdateTableRequestBody.getPolicies().getReplication() != null) { @@ -155,6 +158,18 @@ private void validatePolicies(CreateUpdateTableRequestBody createUpdateTableRequ replicationConfigValidator.getMessage()))); } } + if (createUpdateTableRequestBody.getPolicies() != null + && createUpdateTableRequestBody.getPolicies().getHistory() != null) { + if (!historyPolicySpecValidator.validate( + createUpdateTableRequestBody.getPolicies().getHistory(), tableUri)) { + throw new RequestValidationFailureException( + Arrays.asList( + String.format( + "%s : %s", + historyPolicySpecValidator.getField(), + historyPolicySpecValidator.getMessage()))); + } + } } @SuppressWarnings("checkstyle:OperatorWrap") diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/PoliciesSpecValidator.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/RetentionPolicySpecValidator.java similarity index 96% rename from services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/PoliciesSpecValidator.java rename to services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/RetentionPolicySpecValidator.java index 5f37743b6..3fd00e37a 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/PoliciesSpecValidator.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/RetentionPolicySpecValidator.java @@ -13,12 +13,12 @@ import org.springframework.stereotype.Component; /** - * PoliciesSpecValidator is a custom validator to validate the input values for period in retention - * policy. This custom validator can be used to add validators for fields in policies + * RetentionPolicySpecValidator is a custom validator to validate the input values for period in + * retention policy. */ @Component @Slf4j -public class PoliciesSpecValidator { +public class RetentionPolicySpecValidator { private String failureMessage = ""; diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidatorTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidatorTest.java new file mode 100644 index 000000000..606fa1d8b --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/HistoryPolicySpecValidatorTest.java @@ -0,0 +1,131 @@ +package com.linkedin.openhouse.tables.api.validator.impl; + +import com.linkedin.openhouse.common.api.spec.TableUri; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.History; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.TimePartitionSpec; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class HistoryPolicySpecValidatorTest { + HistoryPolicySpecValidator validator; + + TableUri tableUri = TableUri.builder().build(); + + @BeforeEach + public void setup() { + this.validator = new HistoryPolicySpecValidator(); + } + + @Test + void testValidateRejectsUnstructuredmaxAge() { + History historyWithNoGranularity = History.builder().maxAge(1).build(); + + Assertions.assertFalse(this.validator.validate(historyWithNoGranularity, tableUri)); + Assertions.assertTrue(this.validator.getMessage().contains("Incorrect maxAge specified")); + + History historyWithNomaxAge = + History.builder().granularity(TimePartitionSpec.Granularity.DAY).versions(3).build(); + + Assertions.assertFalse(this.validator.validate(historyWithNomaxAge, tableUri)); + Assertions.assertTrue(this.validator.getMessage().contains("Incorrect maxAge specified")); + } + + @Test + void testValidateDefineNonNullRetentionPolicies() { + History history = History.builder().build(); + + Assertions.assertFalse(this.validator.validate(history, tableUri)); + Assertions.assertTrue( + this.validator + .getMessage() + .contains("Must define either a time based retention or count based retention")); + } + + @Test + void testValidateHistoryMaximums() { + // Exceed days + History historyDaysExceeded = + History.builder() + .maxAge(4) + .granularity(TimePartitionSpec.Granularity.DAY) + .versions(10) + .build(); + Assertions.assertFalse(this.validator.validate(historyDaysExceeded, tableUri)); + + // Exceed days in hours + History historyHoursExceeded = + History.builder().maxAge(100).granularity(TimePartitionSpec.Granularity.HOUR).build(); + Assertions.assertFalse(this.validator.validate(historyHoursExceeded, tableUri)); + + // Exceed Granularity + History historyGranularityExceeded = + History.builder().maxAge(2).granularity(TimePartitionSpec.Granularity.MONTH).build(); + Assertions.assertFalse(this.validator.validate(historyGranularityExceeded, tableUri)); + + // Exceed version count + History historyCountExceeded = History.builder().versions(1000).build(); + Assertions.assertFalse(this.validator.validate(historyCountExceeded, tableUri)); + + // Exceed both policies + History historyBothExceeded = + History.builder() + .maxAge(100) + .granularity(TimePartitionSpec.Granularity.HOUR) + .versions(1000) + .build(); + Assertions.assertFalse(this.validator.validate(historyBothExceeded, tableUri)); + Assertions.assertTrue(this.validator.getMessage().contains("must be between")); + } + + @Test + void testValidateHistoryMinimums() { + // Less than minimum number of hours + History historyHoursMin = + History.builder().maxAge(1).granularity(TimePartitionSpec.Granularity.HOUR).build(); + Assertions.assertFalse(this.validator.validate(historyHoursMin, tableUri)); + + // Less than 2 versions + History historyVersionsMin = History.builder().versions(1).build(); + Assertions.assertFalse(this.validator.validate(historyVersionsMin, tableUri)); + + // Less than minimum number of days + History historyDaysMin = + History.builder().maxAge(0).granularity(TimePartitionSpec.Granularity.DAY).build(); + Assertions.assertFalse(this.validator.validate(historyDaysMin, tableUri)); + + // Less than both policies + History historyPolicyMin = + History.builder() + .maxAge(5) + .granularity(TimePartitionSpec.Granularity.HOUR) + .versions(1) + .build(); + Assertions.assertFalse(this.validator.validate(historyPolicyMin, tableUri)); + Assertions.assertTrue(this.validator.getMessage().contains("must be between")); + } + + @Test + void testValidatePoliciesPositive() { + // Only define maxAge + History history = + History.builder().maxAge(36).granularity(TimePartitionSpec.Granularity.HOUR).build(); + Assertions.assertTrue(this.validator.validate(history, tableUri)); + + history = History.builder().versions(50).build(); + Assertions.assertTrue(this.validator.validate(history, tableUri)); + + // Only define versions + history = History.builder().versions(10).build(); + Assertions.assertTrue(this.validator.validate(history, tableUri)); + + // Define both maxAge and version count + history = + History.builder() + .maxAge(3) + .granularity(TimePartitionSpec.Granularity.DAY) + .versions(10) + .build(); + Assertions.assertTrue(this.validator.validate(history, tableUri)); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/PoliciesSpecValidatorTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/RetentionPolicySpecValidatorTest.java similarity index 97% rename from services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/PoliciesSpecValidatorTest.java rename to services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/RetentionPolicySpecValidatorTest.java index f150d39e7..06ce988cb 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/PoliciesSpecValidatorTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/impl/RetentionPolicySpecValidatorTest.java @@ -15,9 +15,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -class PoliciesSpecValidatorTest { +class RetentionPolicySpecValidatorTest { - PoliciesSpecValidator validator; + RetentionPolicySpecValidator validator; private Schema dummySchema; @@ -25,7 +25,7 @@ class PoliciesSpecValidatorTest { @BeforeEach public void setup() { - this.validator = new PoliciesSpecValidator(); + this.validator = new RetentionPolicySpecValidator(); this.dummySchema = new Schema( required(1, "id", Types.StringType.get()), required(2, "aa", Types.StringType.get())); @@ -214,7 +214,7 @@ void testValidate() { Field failedMsg = org.springframework.util.ReflectionUtils.findField( - PoliciesSpecValidator.class, "failureMessage"); + RetentionPolicySpecValidator.class, "failureMessage"); Assertions.assertNotNull(failedMsg); org.springframework.util.ReflectionUtils.makeAccessible(failedMsg); Assertions.assertTrue( @@ -237,7 +237,7 @@ void testValidate() { failedMsg = org.springframework.util.ReflectionUtils.findField( - PoliciesSpecValidator.class, "failureMessage"); + RetentionPolicySpecValidator.class, "failureMessage"); Assertions.assertNotNull(failedMsg); org.springframework.util.ReflectionUtils.makeAccessible(failedMsg); Assertions.assertTrue( diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesControllerTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesControllerTest.java index 64ce93a2f..1f114537b 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesControllerTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesControllerTest.java @@ -24,6 +24,7 @@ import com.linkedin.openhouse.housetables.client.model.ToggleStatus; import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateTableRequestBody; import com.linkedin.openhouse.tables.api.spec.v0.request.components.ClusteringColumn; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.History; import com.linkedin.openhouse.tables.api.spec.v0.request.components.Policies; import com.linkedin.openhouse.tables.api.spec.v0.request.components.PolicyTag; import com.linkedin.openhouse.tables.api.spec.v0.request.components.Replication; @@ -1193,4 +1194,74 @@ public void testUpdateSucceedsForMultipleReplicationConfig() throws Exception { RequestAndValidateHelper.deleteTableAndValidateResponse(mvc, GET_TABLE_RESPONSE_BODY); } + + @Test + public void testUpdateSucceedsForHistoryPolicy() throws Exception { + MvcResult mvcResult = + RequestAndValidateHelper.createTableAndValidateResponse( + GET_TABLE_RESPONSE_BODY, mvc, storageManager); + + LinkedHashMap currentPolicies = + JsonPath.read(mvcResult.getResponse().getContentAsString(), "$.policies"); + + History history = + History.builder().maxAge(3).granularity(TimePartitionSpec.Granularity.DAY).build(); + + Policies newPolicies = Policies.builder().history(history).build(); + + GetTableResponseBody container = GetTableResponseBody.builder().policies(newPolicies).build(); + GetTableResponseBody addProp = buildGetTableResponseBody(mvcResult, container); + mvcResult = + mvc.perform( + MockMvcRequestBuilders.put( + String.format( + ValidationUtilities.CURRENT_MAJOR_VERSION_PREFIX + + "/databases/%s/tables/%s", + addProp.getDatabaseId(), + addProp.getTableId())) + .contentType(MediaType.APPLICATION_JSON) + .content(buildCreateUpdateTableRequestBody(addProp).toJson()) + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andReturn(); + + LinkedHashMap updatedPolicies = + JsonPath.read(mvcResult.getResponse().getContentAsString(), "$.policies"); + + Assertions.assertNotEquals(currentPolicies, updatedPolicies); + + Assertions.assertEquals(updatedPolicies.get("history").get("maxAge"), 3); + Assertions.assertEquals(updatedPolicies.get("history").get("granularity"), "DAY"); + + RequestAndValidateHelper.deleteTableAndValidateResponse(mvc, GET_TABLE_RESPONSE_BODY); + } + + @Test + public void testCreateRequestFailsWithInvalidHistoryPolicy() throws Exception { + History history = History.builder().granularity(TimePartitionSpec.Granularity.DAY).build(); + GetTableResponseBody responseBodyWithNullPolicies = + TableModelConstants.buildGetTableResponseBodyWithPolicy( + GET_TABLE_RESPONSE_BODY, Policies.builder().history(history).build()); + + ResultActions rs = + mvc.perform( + MockMvcRequestBuilders.put( + String.format( + ValidationUtilities.CURRENT_MAJOR_VERSION_PREFIX + + "/databases/%s/tables/%s", + responseBodyWithNullPolicies.getDatabaseId(), + responseBodyWithNullPolicies.getTableId())) + .contentType(MediaType.APPLICATION_JSON) + .content(buildCreateUpdateTableRequestBody(responseBodyWithNullPolicies).toJson()) + .accept(MediaType.APPLICATION_JSON)); + + rs.andExpect(jsonPath("$.status", is(equalToIgnoringCase(HttpStatus.BAD_REQUEST.name())))) + .andExpect( + jsonPath( + "$.message", + containsString( + "Must define either a time based retention or count based retention for snapshots in table"))) + .andExpect(jsonPath("$.error", is(equalTo(HttpStatus.BAD_REQUEST.getReasonPhrase())))) + .andReturn(); + } } diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/TablesValidatorTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/TablesValidatorTest.java index 3423fd207..cfafd9b43 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/TablesValidatorTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/TablesValidatorTest.java @@ -10,6 +10,7 @@ import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateTableRequestBody; import com.linkedin.openhouse.tables.api.spec.v0.request.UpdateAclPoliciesRequestBody; import com.linkedin.openhouse.tables.api.spec.v0.request.components.ClusteringColumn; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.History; import com.linkedin.openhouse.tables.api.spec.v0.request.components.Policies; import com.linkedin.openhouse.tables.api.spec.v0.request.components.Replication; import com.linkedin.openhouse.tables.api.spec.v0.request.components.ReplicationConfig; @@ -830,6 +831,53 @@ public void validateCreateTableRequestParamWithValidReplicationInPoliciesObject( .build())); } + @Test + public void validateCreateTableRequestParamWithValidHistoryPoliciesJson() { + assertDoesNotThrow( + () -> + tablesApiValidator.validateCreateTable( + "c", + "d", + CreateUpdateTableRequestBody.builder() + .databaseId("d") + .tableId("t") + .clusterId("c") + .schema(HEALTH_SCHEMA_LITERAL) + .tableProperties(ImmutableMap.of()) + .timePartitioning( + TimePartitionSpec.builder() + .columnName("timestamp") + .granularity(TimePartitionSpec.Granularity.HOUR) + .build()) + .baseTableVersion("base") + .policies(Policies.builder().history(HISTORY_POLICY).build()) + .build())); + } + + @Test + public void validateRejectCreateTableRequestParamWithInvalidHistoryPolicy() { + assertThrows( + RequestValidationFailureException.class, + () -> + tablesApiValidator.validateCreateTable( + "c", + "d", + CreateUpdateTableRequestBody.builder() + .databaseId("d") + .tableId("t") + .clusterId("c") + .schema(HEALTH_SCHEMA_LITERAL) + .tableProperties(ImmutableMap.of()) + .timePartitioning( + TimePartitionSpec.builder() + .columnName("timestamp") + .granularity(TimePartitionSpec.Granularity.HOUR) + .build()) + .policies(Policies.builder().history(History.builder().build()).build()) + .baseTableVersion("base") + .build())); + } + @Test public void validateUpdateAclPoliciesSpecialCharacter() { assertThrows( diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/mapper/PoliciesSpecMapperTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/mapper/PoliciesSpecMapperTest.java index 3462886c4..827c5ddb2 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/mapper/PoliciesSpecMapperTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/mapper/PoliciesSpecMapperTest.java @@ -40,6 +40,15 @@ public void testToPoliciesSpecJson() { Assertions.assertEquals( JsonPath.read(policiesSpec, "$.replication.config[0].interval"), TableModelConstants.TABLE_POLICIES.getReplication().getConfig().get(0).getInterval()); + Assertions.assertEquals( + (Integer) JsonPath.read(policiesSpec, "$.history.maxAge"), + TABLE_POLICIES.getHistory().getMaxAge()); + Assertions.assertEquals( + JsonPath.read(policiesSpec, "$.history.granularity"), + TABLE_POLICIES.getHistory().getGranularity().toString()); + Assertions.assertEquals( + (Integer) JsonPath.read(policiesSpec, "$.history.versions"), + TABLE_POLICIES.getHistory().getVersions()); } @Test diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/model/TableModelConstants.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/model/TableModelConstants.java index 53599acd6..4154aa36d 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/model/TableModelConstants.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/model/TableModelConstants.java @@ -8,6 +8,7 @@ import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateTableRequestBody; import com.linkedin.openhouse.tables.api.spec.v0.request.IcebergSnapshotsRequestBody; import com.linkedin.openhouse.tables.api.spec.v0.request.components.ClusteringColumn; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.History; import com.linkedin.openhouse.tables.api.spec.v0.request.components.Policies; import com.linkedin.openhouse.tables.api.spec.v0.request.components.Replication; import com.linkedin.openhouse.tables.api.spec.v0.request.components.ReplicationConfig; @@ -43,6 +44,7 @@ public final class TableModelConstants { public static RetentionColumnPattern COL_PAT; public static Retention RETENTION_POLICY; public static Replication REPLICATION_POLICY; + public static History HISTORY_POLICY; public static final Retention RETENTION_POLICY_WITH_PATTERN; public static final Retention RETENTION_POLICY_WITH_EMPTY_PATTERN; @@ -65,7 +67,12 @@ public final class TableModelConstants { ArrayList configs = new ArrayList<>(); configs.add(ReplicationConfig.builder().destination("CLUSTER1").interval("12H").build()); REPLICATION_POLICY = Replication.builder().config(configs).build(); - + HISTORY_POLICY = + History.builder() + .maxAge(3) + .granularity(TimePartitionSpec.Granularity.DAY) + .versions(10) + .build(); RETENTION_POLICY_WITH_PATTERN = Retention.builder() .count(3) @@ -80,7 +87,11 @@ public final class TableModelConstants { .build(); TABLE_POLICIES = - Policies.builder().retention(RETENTION_POLICY).replication(REPLICATION_POLICY).build(); + Policies.builder() + .retention(RETENTION_POLICY) + .replication(REPLICATION_POLICY) + .history(HISTORY_POLICY) + .build(); TABLE_POLICIES_COMPLEX = Policies.builder().retention(RETENTION_POLICY_WITH_PATTERN).build(); TABLE_POLICIES_COMPLEX_STRING = "{\n"