phet commented on code in PR #3535:
URL: https://github.com/apache/gobblin/pull/3535#discussion_r940634518
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/TimeAwareRecursiveCopyableDataset.java:
##########
@@ -68,7 +68,7 @@ public TimeAwareRecursiveCopyableDataset(FileSystem fs, Path
rootPath, Propertie
this.datePattern = properties.getProperty(DATE_PATTERN_KEY);
this.currentTime = properties.containsKey(DATE_PATTERN_TIMEZONE_KEY) ?
LocalDateTime.now(
- DateTimeZone.forID(DATE_PATTERN_TIMEZONE_KEY))
+ DateTimeZone.forID(properties.getProperty(DATE_PATTERN_TIMEZONE_KEY)))
: LocalDateTime.now(DateTimeZone.forID(DEFAULT_DATE_PATTERN_TIMEZONE));
Review Comment:
I find it written most succinctly in -
https://github.com/apache/gobblin/blob/d9ae5353c74fdcd385835fca9b586b3fdb90971b/gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/UnixTimestampRecursiveCopyableDataset.java#L76
BTW, these two could also be DRYed up:
https://github.com/apache/gobblin/blob/d9ae5353c74fdcd385835fca9b586b3fdb90971b/gobblin-data-management/src/main/java/org/apache/gobblin/data/management/version/finder/DatePartitionHiveVersionFinder.java#L69
https://github.com/apache/gobblin/blob/d9ae5353c74fdcd385835fca9b586b3fdb90971b/gobblin-data-management/src/main/java/org/apache/gobblin/data/management/version/finder/DateTimeDatasetVersionFinder.java#L86
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/TimeAwareRecursiveCopyableDatasetTest.java:
##########
@@ -260,6 +260,44 @@ public void testGetFilesAtPath() throws IOException {
}
}
+ @Test
+ public void testTimezoneProperty() throws IOException {
+ // Test in UTC instead of default time
+ String datePattern = "yyyy/MM/dd/HH";
+ DateTimeFormatter formatter = DateTimeFormat.forPattern(datePattern);
+ // Ensure that the files are created in UTC time
+ LocalDateTime endDate = LocalDateTime.now(DateTimeZone.forID("UTC"));
+
+ Set<String> candidateFiles = new HashSet<>();
+ for (int i = 0; i < MAX_NUM_HOURLY_DIRS; i++) {
+ String startDate = endDate.minusHours(i).toString(formatter);
+ Path subDirPath = new Path(baseDir1, new Path(startDate));
+ fs.mkdirs(subDirPath);
+ Path filePath = new Path(subDirPath, i + ".avro");
+ fs.create(filePath);
+ if (i < (NUM_LOOKBACK_HOURS + 1)) {
+ candidateFiles.add(filePath.toString());
+ }
+ }
+
+ //Lookback time = "4h"
+ Properties properties = new Properties();
+
properties.setProperty(TimeAwareRecursiveCopyableDataset.LOOKBACK_TIME_KEY,
NUM_LOOKBACK_HOURS_STR);
+ properties.setProperty(TimeAwareRecursiveCopyableDataset.DATE_PATTERN_KEY,
"yyyy/MM/dd/HH");
+
properties.setProperty(TimeAwareRecursiveCopyableDataset.DATE_PATTERN_TIMEZONE_KEY,
"UTC");
Review Comment:
not major, but right now the impl should throw `IllegalArgExc` when the
timezone is unrecognized. ...but at first I wondered whether UTC wouldn't be a
default it silently uses.
to demonstrate that test success couldn't be due merely to a default of UTC,
would it be worth verifying that `IllegalArgE` gets thrown on a bogus value of
this property?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
