[GOBBLIN-200] Add cleanable state store dataset #2097

jack-moseley · 2017-09-08T20:25:37Z

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. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-200

Description

Here are some details about my PR, including screenshots (if applicable):

Create wrappers for DatasetStoreDataset and DatasetStoreDatasetFinder to make them Cleanable

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Created CleanableDatasetStoreDatasetTest

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"

jack-moseley · 2017-09-08T20:25:58Z

@ibuenros please review

yukuai518 · 2017-09-18T01:27:48Z

...ain/java/org/apache/gobblin/data/management/version/TimestampedDatasetStateStoreVersion.java

+    return this.version.equals(otherAsDateTime.version) ? 0 : this.version.compareTo(otherAsDateTime.version);
+  }
+
+  @Override


You can remove this equal() if the behavior is the same as parent class

Removing this triggers findbugs warning, which recommends explicitly overriding even if behaviour is the same.

yukuai518 · 2017-09-18T01:58:50Z

.../java/org/apache/gobblin/data/management/retention/dataset/CleanableDatasetStoreDataset.java

+ * A cleanable {@link DatasetStoreDataset}
+ */
+@Data
+public class CleanableDatasetStoreDataset extends ModificationTimeDataset {


You probably don't want to assume CleanableDatasetStoreDataset always use time based strategy. . You probably should rename it to ModificationTimeDatasoreDataset if only modification time is used.

It seems like the ModificationTimeDataset contains many FileSystem specific constructs which doesn't represent all the datasets of DatasetStore type, which could be many other types like mysql DB, FS, or other implementations.

I would recommend to keep your first commit, and create your own interface like CleanableDatasetStore, which is similar to CleanableDataset but excluding the inheritance from FileSystemDataset.

Then your CleanableDatasetStoreDataset can implement CleanableDatasetStore ::clean(), which is similar to MultiVersionCleanableDatasetBase::clean (), but can be simplified that VersionFinder is always DatasetStateStoreVersion type. The DatasetStateStoreVersion is an abstract class which takes DatasetStateStoreEntryManager as a constructor argument and serve as a base class of all the version types related to dataset state store

yukuai518 · 2017-09-18T04:08:24Z

gobblin-metastore/src/main/java/org/apache/gobblin/metastore/DatasetStoreDatasetFinder.java

@@ -39,7 +40,7 @@
 /**
 * A {@link DatasetsFinder} to find {@link DatasetStoreDataset}s.
 */
-public class DatasetStoreDatasetFinder implements DatasetsFinder<DatasetStoreDataset> {
+public class DatasetStoreDatasetFinder implements DatasetsFinder<Dataset> {


I feel it's better to limit the type to DatasetStoreDataset here. However this will cause some incompatibility if you want to reuse the existing ModificationTimeDataset. That's why I don't like this class to be derived from ModificationTimeDataset. Please check with Issac to see if this type restriction can be removed. Otherwise you can define your own CleanableDatasetStoreDataset like I mentioned earlier which should always be DatasetStoreDataset type.

yukuai518 · 2017-09-18T04:19:35Z

.../java/org/apache/gobblin/data/management/retention/dataset/CleanableDatasetStoreDataset.java

+  @Override
+  protected void cleanImpl(Collection deletableVersions) throws IOException {
+    for (Object version : deletableVersions) {
+      ((TimestampedDatasetStateStoreVersion) version).getEntry().delete();


I think this logic can be generic (apply to all datastore datasets), so we can avoid the downcast type conversion.

yukuai518 · 2017-09-18T05:48:47Z

.../java/org/apache/gobblin/data/management/retention/dataset/CleanableDatasetStoreDataset.java

+ * A cleanable {@link DatasetStoreDataset}
+ */
+@Data
+public class CleanableDatasetStoreDataset extends ModificationTimeDataset {


It seems like the ModificationTimeDataset contains many FileSystem specific constructs which doesn't represent all the datasets of DatasetStore type, which could be many other types like mysql DB, FS, or other implementations.

I would recommend to keep your first commit, and create your own interface like CleanableDatasetStore, which is similar to CleanableDataset but excluding the inheritance from FileSystemDataset.

Then your CleanableDatasetStoreDataset can implement CleanableDatasetStore ::clean(), which is similar to MultiVersionCleanableDatasetBase::clean (), but can be simplified that VersionFinder is always DatasetStateStoreVersion type. The DatasetStateStoreVersion is an abstract class which takes DatasetStateStoreEntryManager as a constructor argument and serve as a base class of all the version types related to dataset state store

yukuai518 · 2017-09-18T05:56:36Z

...ain/java/org/apache/gobblin/data/management/version/TimestampedDatasetStateStoreVersion.java

+ * {@link TimestampedDatasetVersion} that has a {@link DatasetStateStoreEntryManager}
+ */
+@Getter
+public class TimestampedDatasetStateStoreVersion extends TimestampedDatasetVersion {


TimestampedDatasetVersion is FileSystem typed. Consider to have a base class and make TimestampedDatasetStateStoreVersion as a derived class.

yukuai518 · 2017-09-18T06:06:44Z

gobblin-metastore/src/main/java/org/apache/gobblin/metastore/DatasetStoreDatasetFinder.java

@@ -76,7 +77,7 @@ private StateStorePredicate buildPredicate() {
  }

  @Override
-  public List<DatasetStoreDataset> findDatasets() throws IOException {
+  public List<Dataset> findDatasets() throws IOException {


Similar problem.

…inder/policy

jack-moseley · 2017-09-26T01:44:01Z

@yukuai518 refactored based on your comments:

Removed CleanableDataset dependency on FileSystem
Changed CleanableDatasetStoreDataset to inherit directly from CleanableDataset
Changed CleanableDatasetStoreDataset to be an abstract base class where subclasses can choose which VersionFinder/Policy to use
Added DatasetStateStoreVersion

yukuai518

LGTM

ibuenros · 2017-09-28T16:42:08Z

.../java/org/apache/gobblin/data/management/retention/dataset/CleanableDatasetStoreDataset.java

+
+    Collections.sort(versions, Collections.reverseOrder());
+
+    Collection<T> deletableVersions = this.getVersionSelectionPolicy().listSelectedVersions(versions);


Shouldn't this be the other way around? VersionSelectionPolicy selects the versions to keep.

It could be used as versions to delete as well, and MultiVersionCleanableDatasetBase does it this way.

ibuenros · 2017-09-28T16:43:16Z

...ement/src/main/java/org/apache/gobblin/data/management/version/DatasetStateStoreVersion.java

+/**
+ * {@link DatasetVersion} that has a {@link DatasetStateStoreEntryManager}
+ */
+public interface DatasetStateStoreVersion extends DatasetVersion {


What is the point of this class?

Allows CleanableDatasetStoreDataset to call getEntry()

ibuenros · 2017-09-29T17:38:35Z

+1

Closes apache#2097 from jack-moseley/state_store_cleaner

Add cleanable state store dataset

9d369e5

ibuenros approved these changes Sep 11, 2017

View reviewed changes

jack-moseley added 2 commits September 12, 2017 14:01

Add DatasetStoreDataset cleaning by timestamp

bd3899c

Add example config

6f3f85b

yukuai518 suggested changes Sep 18, 2017

View reviewed changes

jack-moseley added 2 commits September 25, 2017 18:02

Refactor to remove dependency on FileSystem and abstract out VersionF…

91fd711

…inder/policy

Fix findbugs warning

968387e

yukuai518 approved these changes Sep 26, 2017

View reviewed changes

ibuenros suggested changes Sep 28, 2017

View reviewed changes

ibuenros approved these changes Sep 29, 2017

View reviewed changes

asfgit closed this in 70cbe91 Sep 29, 2017

zxliucmu pushed a commit to zxliucmu/incubator-gobblin that referenced this pull request Nov 16, 2017

[GOBBLIN-200] Add cleanable state store dataset

0b13db6

Closes apache#2097 from jack-moseley/state_store_cleaner

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-200] Add cleanable state store dataset #2097

[GOBBLIN-200] Add cleanable state store dataset #2097

jack-moseley commented Sep 8, 2017

jack-moseley commented Sep 8, 2017

yukuai518 Sep 18, 2017 •

edited

jack-moseley Sep 26, 2017

yukuai518 Sep 18, 2017 •

edited

yukuai518 Sep 18, 2017

yukuai518 Sep 18, 2017

yukuai518 Sep 18, 2017

yukuai518 Sep 18, 2017

yukuai518 Sep 18, 2017

yukuai518 Sep 18, 2017

jack-moseley commented Sep 26, 2017

yukuai518 left a comment

ibuenros Sep 28, 2017

jack-moseley Sep 28, 2017

ibuenros Sep 28, 2017

jack-moseley Sep 28, 2017

ibuenros commented Sep 29, 2017


		Collections.sort(versions, Collections.reverseOrder());

		Collection<T> deletableVersions = this.getVersionSelectionPolicy().listSelectedVersions(versions);

[GOBBLIN-200] Add cleanable state store dataset #2097

[GOBBLIN-200] Add cleanable state store dataset #2097

Conversation

jack-moseley commented Sep 8, 2017

JIRA

Description

Tests

Commits

jack-moseley commented Sep 8, 2017

yukuai518 Sep 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukuai518 Sep 18, 2017 • edited

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

jack-moseley commented Sep 26, 2017

yukuai518 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

ibuenros commented Sep 29, 2017

yukuai518 Sep 18, 2017 •

edited

yukuai518 Sep 18, 2017 •

edited