sv2000 commented on a change in pull request #3178:
URL: https://github.com/apache/incubator-gobblin/pull/3178#discussion_r548225860
##########
File path:
gobblin-api/src/main/java/org/apache/gobblin/dataset/DatasetDescriptor.java
##########
@@ -17,34 +17,52 @@
package org.apache.gobblin.dataset;
-import java.util.Map;
-
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
-
import lombok.Getter;
+import javax.annotation.Nullable;
+import java.util.Map;
+import java.util.Objects;
+
/**
* A {@link Descriptor} identifies and provides metadata to describe a dataset
*/
public class DatasetDescriptor extends Descriptor {
private static final String PLATFORM_KEY = "platform";
private static final String NAME_KEY = "name";
+ private static final String CLUSTER_NAME_KEY = "clusterName";
/**
* which platform the dataset is stored, for example: local, hdfs, oracle,
mysql, kafka
*/
@Getter
private final String platform;
+ /**
+ * Human-readeable cluster name.
+ *
+ * @see org.apache.gobblin.util.ClustersNames
+ */
+ @Getter
+ @Nullable
+ private final String clusterName;
Review comment:
What exactly does clusterName mean? Is this the name of the storage
instance that has an instance of a given dataset? If so, I think
storageInstance or storageInstanceName would better capture what this field is.
##########
File path:
gobblin-api/src/main/java/org/apache/gobblin/dataset/DatasetDescriptor.java
##########
@@ -17,34 +17,52 @@
package org.apache.gobblin.dataset;
-import java.util.Map;
-
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
-
import lombok.Getter;
+import javax.annotation.Nullable;
+import java.util.Map;
+import java.util.Objects;
+
/**
* A {@link Descriptor} identifies and provides metadata to describe a dataset
*/
public class DatasetDescriptor extends Descriptor {
Review comment:
One comment: it is not clear from this class if it was intended to
capture the notion of a Dataset instance. If so, there might be advantages to
extending this class to a new descriptor that defines a dataset instance where
the "clusterName" is mandatory.
##########
File path:
gobblin-api/src/main/java/org/apache/gobblin/dataset/DatasetDescriptor.java
##########
@@ -17,34 +17,52 @@
package org.apache.gobblin.dataset;
-import java.util.Map;
-
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
-
import lombok.Getter;
+import javax.annotation.Nullable;
+import java.util.Map;
+import java.util.Objects;
+
/**
* A {@link Descriptor} identifies and provides metadata to describe a dataset
*/
public class DatasetDescriptor extends Descriptor {
private static final String PLATFORM_KEY = "platform";
private static final String NAME_KEY = "name";
+ private static final String CLUSTER_NAME_KEY = "clusterName";
/**
* which platform the dataset is stored, for example: local, hdfs, oracle,
mysql, kafka
*/
@Getter
private final String platform;
+ /**
+ * Human-readeable cluster name.
+ *
+ * @see org.apache.gobblin.util.ClustersNames
+ */
+ @Getter
+ @Nullable
+ private final String clusterName;
Review comment:
What is the expected format for the clusterName? Can it be any string? I
think it would be better to define a new class called "StorageIdentifier" that
includes among other things fields such as logicalName, address, port,
availability zone, etc. We should also define a URN representation for the
storage instance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
