You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@gobblin.apache.org by "jack-moseley (via GitHub)" <gi...@apache.org> on 2023/04/11 00:32:08 UTC

[GitHub] [gobblin] jack-moseley opened a new pull request, #3670: [GOBBLIN-1809] Add new lookback version finder for use with iceberg retention

jack-moseley opened a new pull request, #3670:
URL: https://github.com/apache/gobblin/pull/3670

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

### JIRA
- [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1809

### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
Add new version finder which does not actually list filepaths, and just constructs all unique path names/versions for a given lookback time with a given path partition pattern. This is useful for example with the iceberg cleanable datasets, which if decoupled from data files, should not depend on existence of the file path.

Also fixed a bug in `DatasetCleaner` which sometimes causes the job to fail during shutdown and not print the actual issue with the job.

### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Added unit test and tested in retention job

### Commits
- [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] homatthew commented on a diff in pull request #3670: [GOBBLIN-1809] Add new lookback version finder for use with iceberg retention

Posted by "homatthew (via GitHub)" <gi...@apache.org>.

homatthew commented on code in PR #3670:
URL: https://github.com/apache/gobblin/pull/3670#discussion_r1162191798


##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/version/finder/LookbackDateTimeDatasetVersionFinderTest.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.data.management.version.finder;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.joda.time.DateTimeZone;
+import org.joda.time.Instant;
+import org.joda.time.format.DateTimeFormat;
+import org.joda.time.format.DateTimeFormatter;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.data.management.version.TimestampedDatasetVersion;
+import org.apache.gobblin.dataset.Dataset;
+import org.apache.gobblin.dataset.FileSystemDataset;
+import org.apache.gobblin.util.ConfigUtils;
+
+
+@Test(groups = { "gobblin.data.management.version" })
+public class LookbackDateTimeDatasetVersionFinderTest {
+
+  private FileSystem fs;
+  private DateTimeFormatter formatter = DateTimeFormat.forPattern("yyyy/MM/dd/HH").withZone(DateTimeZone.forID(ConfigurationKeys.PST_TIMEZONE_NAME));
+  // Fixed time for test consistency: April 10, 2023 5:01:27 PM GMT-07:00
+  private final Instant fixedTime = new Instant(1681171287000L);

Review Comment:
   Consider using  instead of the comment + Epoch time
   ```
    final Instant frozenInstant = Instant.parse("2023-01-01T00:00:00.000Z");
   ```



##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/version/finder/LookbackDateTimeDatasetVersionFinderTest.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.data.management.version.finder;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.joda.time.DateTimeZone;
+import org.joda.time.Instant;
+import org.joda.time.format.DateTimeFormat;
+import org.joda.time.format.DateTimeFormatter;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.data.management.version.TimestampedDatasetVersion;
+import org.apache.gobblin.dataset.Dataset;
+import org.apache.gobblin.dataset.FileSystemDataset;
+import org.apache.gobblin.util.ConfigUtils;
+
+
+@Test(groups = { "gobblin.data.management.version" })
+public class LookbackDateTimeDatasetVersionFinderTest {
+
+  private FileSystem fs;
+  private DateTimeFormatter formatter = DateTimeFormat.forPattern("yyyy/MM/dd/HH").withZone(DateTimeZone.forID(ConfigurationKeys.PST_TIMEZONE_NAME));
+  // Fixed time for test consistency: April 10, 2023 5:01:27 PM GMT-07:00
+  private final Instant fixedTime = new Instant(1681171287000L);

Review Comment:
   Consider using Instant.parse instead of the comment + Epoch time
   ```
    final Instant frozenInstant = Instant.parse("2023-01-01T00:00:00.000Z");
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] homatthew commented on a diff in pull request #3670: [GOBBLIN-1809] Add new lookback version finder for use with iceberg retention

Posted by "homatthew (via GitHub)" <gi...@apache.org>.

homatthew commented on code in PR #3670:
URL: https://github.com/apache/gobblin/pull/3670#discussion_r1163363722


##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/version/finder/LookbackDateTimeDatasetVersionFinder.java:
##########
@@ -67,25 +66,20 @@ public LookbackDateTimeDatasetVersionFinder(FileSystem fs, Config config, Instan
     Preconditions.checkArgument(config.hasPath(VERSION_LOOKBACK_PERIOD) , "Missing required property " + VERSION_LOOKBACK_PERIOD);
     PeriodFormatter periodFormatter =
         new PeriodFormatterBuilder().appendYears().appendSuffix("y").appendMonths().appendSuffix("M").appendDays()
-            .appendSuffix("d").appendHours().appendSuffix("h").appendMinutes().appendSuffix("m").toFormatter();
-    this.stepDuration = Duration.standardMinutes(1);
+            .appendSuffix("d").appendHours().appendSuffix("h").toFormatter();
+    this.stepDuration = Duration.standardHours(1);
     this.pathPrefix = ConfigUtils.getString(config, VERSION_PATH_PREFIX, "");
     this.lookbackPeriod = periodFormatter.parsePeriod(config.getString(VERSION_LOOKBACK_PERIOD));
     this.endTime = endTime;
   }
 
-  @Override
-  public Class<? extends FileSystemDatasetVersion> versionClass() {
-    return TimestampedDatasetVersion.class;
-  }
-
   @Override
   public Collection<TimestampedDatasetVersion> findDatasetVersions(Dataset dataset) throws IOException {
     FileSystemDataset fsDataset = (FileSystemDataset) dataset;
     Set<TimestampedDatasetVersion> versions = new HashSet<>();
     Instant startTime = endTime.minus(lookbackPeriod.toStandardDuration());
 
-    while (startTime.isBefore(endTime)) {
+    while (!startTime.isAfter(endTime)) {

Review Comment:
   Does making this a for loop make it easier to read? E.g.
   ```
   for (Instant time = startTime; !time.isAfter(endTime); time.plus(stepDuration))
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org