[GOBBLIN-1001] Implement TimePartitionGlobFinder #2846

zxcware · 2019-12-10T18:20:56Z

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

My PR addresses the following Gobblin JIRA issues and references them in the PR title.
- https://issues.apache.org/jira/browse/GOBBLIN-1001

Description

Here are some details about my PR:
- A TimePartitionGlobFinder has the capability to create empty file system dataset if a partition has empty data

Tests

My PR adds the following unit tests:
- TimePartitionGlobFinder.testDayPartition covers an empty day partition dataset is created when TimePartitionGlobFinder.enableEmptyPartition is true.

Commits

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":
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"

codecov-io · 2019-12-10T19:11:20Z

Codecov Report

Merging #2846 into master will decrease coverage by 0.02%.
The diff coverage is 88.33%.

@@             Coverage Diff              @@
##             master    #2846      +/-   ##
============================================
- Coverage     45.59%   45.57%   -0.03%     
- Complexity     8984     9031      +47     
============================================
  Files          1904     1908       +4     
  Lines         71347    71729     +382     
  Branches       7876     7912      +36     
============================================
+ Hits          32534    32692     +158     
- Misses        35806    36031     +225     
+ Partials       3007     3006       -1

Impacted Files	Coverage Δ	Complexity Δ
...gement/copy/TimeAwareRecursiveCopyableDataset.java	`91.17% <ø> (ø)`	`15 <0> (ø)`	⬇️
...anagement/dataset/DefaultFileSystemGlobFinder.java	`100% <100%> (+100%)`	`2 <1> (+2)`	⬆️
...ain/java/org/apache/gobblin/time/TimeIterator.java	`63.15% <63.15%> (ø)`	`4 <4> (?)`
...ta/management/dataset/SimpleFileSystemDataset.java	`88.88% <88.88%> (ø)`	`4 <4> (?)`
...ta/management/dataset/TimePartitionGlobFinder.java	`93.4% <93.4%> (ø)`	`23 <23> (?)`
...rg/apache/gobblin/salesforce/SalesforceSource.java	`19.81% <0%> (-2.95%)`	`12% <0%> (+1%)`
...in/java/org/apache/gobblin/cluster/HelixUtils.java	`32.71% <0%> (-2.81%)`	`11% <0%> (-1%)`
...service/modules/orchestration/DagManagerUtils.java	`85.54% <0%> (-1.21%)`	`37% <0%> (ø)`
...main/java/org/apache/gobblin/yarn/YarnService.java	`14.84% <0%> (-0.85%)`	`3% <0%> (-1%)`
...nversion/hive/query/HiveAvroORCQueryGenerator.java	`64.51% <0%> (-0.27%)`	`85% <0%> (+1%)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a1debc...2d5c8f4. Read the comment docs.

autumnust

Actually why not using existing Glob finder with your time-partition pattern instead of inventing another finder and did you search through other alternatives ?
I don't remember exactly the finder but I have rough impression something else serve the same purpose, just curious

zxcware · 2019-12-11T01:16:15Z

@autumnust It does use, compositely use instead of inheriting, an existing GlobFinder, which is DefaultFileSystemGlobFinder.

autumnust · 2019-12-11T01:53:04Z

@autumnust It does use, compositely use instead of inheriting, an existing GlobFinder, which is DefaultFileSystemGlobFinder.

OK Gotcha. If enableEmptyPartition is not enabled, it does nothing more than just setting a different pattern using DefaultFileSystemGlobFinder. If it does, it somehow creates empty dataset for yesterday's partitions. This doesn't seem to be a generic finder to me but serving a very specific purpose. Would you consider doing it as a separated extension for GlobFinder internally instead of open-source?

zxcware · 2019-12-11T21:28:23Z

@autumnust Yeah, yesterdayPartition is really specific, I'm thinking about generalize it to enforcePreviousN(looking for better name suggestions) partitions. Its main responsibility is to create EmptyFileSystemDataset if any of the previous N doesn't exist, signaling quiet time. In addition, it focuses on time partitions and supports different time formats(not limitted to yyyy/MM/dd) compared to vanilla DefaultFileSystemGlobFinder. (I'm adding comments about it s usage)

By enforcePreviousN, it's tied with company requirements even less and makes it more justifiable to open-source. In our use case, we capture the quiet time signal to publish compaction watermark. It can be captured by others to do different operations.

Another consideration was we have to make internal copies of open source compaction constructs(MRTask, Verifier, CompactionAction), if EmptyFileSystemDataset is made internal. Compared to make EmptyFileSystemDataset first citizen of open source compaction flow, the implementation and mountainous cost of internalization is high, given most of our pipelines use open source compaction constructs

autumnust

Discussed offline about the context and use-case, agreed on the approach to implement.
Several comments.

autumnust · 2019-12-12T18:51:59Z

...nt/src/main/java/org/apache/gobblin/data/management/dataset/DefaultFileSystemGlobFinder.java

@@ -33,16 +33,6 @@ public DefaultFileSystemGlobFinder(FileSystem fs, Properties properties) throws
  }

  public FileSystemDataset datasetAtPath(final Path path) throws IOException {
-    return new FileSystemDataset() {


There are more than one places that having this anonymous class impl. Shall we refactor them all if you would like to replace it with the real impl. ?

Fixed PinotAuditCountVerifierTest. Its usage in CompactionSuiteBase will be replaced by a new ser/de mechanism.

autumnust · 2019-12-12T18:52:54Z

...agement/src/main/java/org/apache/gobblin/data/management/dataset/EmptyFileSystemDataset.java

+import org.apache.hadoop.fs.Path;
+
+
+public class EmptyFileSystemDataset extends SimpleFileSystemDataset {


This looks like an overkill if only thing you want is a different class. Shall we add a flag field within SimpleFileSystemDataset to identify an emptyDataset instead ?

autumnust · 2019-12-12T18:53:20Z

...gement/src/main/java/org/apache/gobblin/data/management/dataset/SimpleFileSystemDataset.java

+import org.apache.gobblin.dataset.FileSystemDataset;
+
+
+public class SimpleFileSystemDataset implements FileSystemDataset {


Please add javadoc for the reasons why we need it

autumnust · 2019-12-12T18:56:12Z

...gement/src/main/java/org/apache/gobblin/data/management/dataset/TimePartitionGlobFinder.java

+
+
+@Slf4j
+public class TimePartitionGlobFinder implements DatasetsFinder<FileSystemDataset> {


Please add javadoc to differentiate it with DefaultFileSystemGlobFinder is you decide not to override

autumnust · 2019-12-12T19:04:48Z

...gement/src/main/java/org/apache/gobblin/data/management/dataset/TimePartitionGlobFinder.java

+    datasets.forEach(dataset -> yesterdayDatasetPartitions.add(createYesterdayDatasetPartition(dataset)));
+
+    // Find all dataset time partitions
+    List<FileSystemDataset> datasetPartitions = findDatasets(datasetPartitionPattern);


Could this code block be optimized to be a single expression like :
datasetPartitions.addAll(yesterdaypartitions.stream().allMatch(x->!datasetPartitions.contains(x)).map(x -> createEmptyFileDataset)) ?

zxcware · 2019-12-14T01:35:05Z

@autumnust I generalized the finder which supports look back and will create a virtual partition dataset for any partition within the look back window that doesn't have a physical folder

autumnust

Minor comments

autumnust · 2019-12-16T18:39:52Z

...gement/src/main/java/org/apache/gobblin/data/management/dataset/TimePartitionGlobFinder.java

+        ZonedDateTime.now(ZoneId.of(properties.getProperty(TIME_ZONE, DEFAULT_TIME_ZONE))));
+  }
+
+  @VisibleForTesting


From what I learned before, this annotation is not helping you testing methods and it doesn't even make the annotated method loaded in JVM: https://stackoverflow.com/questions/24051476/guava-visiblefortesting-help-me-with-a-complete-example it is mostly used for documentation purpose.

Can you double-check if you need it?

yeah, I learned it and meant to use it for documentation purpose if we haven't found an alternative way

autumnust · 2019-12-16T18:56:03Z

...gement/src/main/java/org/apache/gobblin/data/management/dataset/TimePartitionGlobFinder.java

+    List<FileSystemDataset> actualPartitions = findDatasets(datasetPartitionPattern);
+
+    String pathStr;
+    for (FileSystemDataset physicalPartition : actualPartitions) {


Isn't this for-loop a intersection operation of actualPartitions and computedPartitions with a tweak on element type ?

Right. The loop also results in the diff between computedPartitions and actualPartitions

autumnust

LGTM overall

autumnust · 2019-12-20T19:33:06Z

...gement/src/main/java/org/apache/gobblin/data/management/dataset/SimpleFileSystemDataset.java

@@ -46,6 +50,9 @@ public String datasetURN() {
    return path.toString();
  }

+  /**
+   * @return true if the dataset doesn't have a physical file/folder
+   */
  public boolean getIsVirtual() {


Does it sound more intuitive if isVirtual ?

htran1

+1

Closes apache#2846 from zxcware/comp

zxcware added 2 commits December 10, 2019 10:17

[GOBBLIN-1001] Implement TimePartitionGlobFinder

5d2cc7b

Add test file and apache licenses

06403c7

autumnust reviewed Dec 11, 2019

View reviewed changes

autumnust requested changes Dec 12, 2019

View reviewed changes

A more general TimePartitionGlobFinder

de6c2da

autumnust reviewed Dec 16, 2019

View reviewed changes

zxcware added 3 commits December 17, 2019 17:24

Comment DateRangeIterator

f815865

Fix derive partition bugs

f8f9c54

Add comments

2d5c8f4

autumnust approved these changes Dec 20, 2019

View reviewed changes

htran1 approved these changes Dec 21, 2019

View reviewed changes

asfgit closed this in 0700092 Dec 21, 2019

zxcware deleted the comp branch February 7, 2020 01:11

haojiliu pushed a commit to haojiliu/incubator-gobblin that referenced this pull request Apr 9, 2020

[GOBBLIN-1001] Implement TimePartitionGlobFinder

f89d026

Closes apache#2846 from zxcware/comp

jhsenjaliya pushed a commit to jhsenjaliya/incubator-gobblin that referenced this pull request Apr 26, 2020

[GOBBLIN-1001] Implement TimePartitionGlobFinder

ff99896

Closes apache#2846 from zxcware/comp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-1001] Implement TimePartitionGlobFinder #2846

[GOBBLIN-1001] Implement TimePartitionGlobFinder #2846

zxcware commented Dec 10, 2019 •

edited

codecov-io commented Dec 10, 2019 •

edited

autumnust left a comment

zxcware commented Dec 11, 2019

autumnust commented Dec 11, 2019

zxcware commented Dec 11, 2019

autumnust left a comment

autumnust Dec 12, 2019

zxcware Dec 14, 2019

autumnust Dec 12, 2019

autumnust Dec 12, 2019

autumnust Dec 12, 2019

autumnust Dec 12, 2019

zxcware commented Dec 14, 2019

autumnust left a comment

autumnust Dec 16, 2019

zxcware Dec 16, 2019 •

edited

autumnust Dec 16, 2019

zxcware Dec 16, 2019

autumnust left a comment

autumnust Dec 20, 2019

htran1 left a comment

		import org.apache.hadoop.fs.Path;


		public class EmptyFileSystemDataset extends SimpleFileSystemDataset {

		import org.apache.gobblin.dataset.FileSystemDataset;


		public class SimpleFileSystemDataset implements FileSystemDataset {



		@Slf4j
		public class TimePartitionGlobFinder implements DatasetsFinder<FileSystemDataset> {

[GOBBLIN-1001] Implement TimePartitionGlobFinder #2846

[GOBBLIN-1001] Implement TimePartitionGlobFinder #2846

Conversation

zxcware commented Dec 10, 2019 • edited

JIRA

Description

Tests

Commits

codecov-io commented Dec 10, 2019 • edited

Codecov Report

autumnust left a comment

Choose a reason for hiding this comment

zxcware commented Dec 11, 2019

autumnust commented Dec 11, 2019

zxcware commented Dec 11, 2019

autumnust left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxcware commented Dec 14, 2019

autumnust left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxcware Dec 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autumnust left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htran1 left a comment

Choose a reason for hiding this comment

zxcware commented Dec 10, 2019 •

edited

codecov-io commented Dec 10, 2019 •

edited

zxcware Dec 16, 2019 •

edited