You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2021/08/04 14:13:25 UTC
[GitHub] [gobblin] abhisheknath2011 opened a new pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
abhisheknath2011 opened a new pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349
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](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-XXX
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
- [ ] Replaced deprecated public APIs with the ones compatible with Avro 1.9.
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### 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](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] abhisheknath2011 commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
abhisheknath2011 commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r686874892
##########
File path: gobblin-core-base/src/main/java/org/apache/gobblin/test/SequentialTestSource.java
##########
@@ -29,7 +29,7 @@
import com.typesafe.config.Config;
import com.typesafe.config.ConfigFactory;
-import avro.shaded.com.google.common.base.Throwables;
+import com.google.common.base.Throwables;
Review comment:
We already have guava library dependency and this import can be directly referred from there. Avro 1.9 is lean and it has removed shaded guava library. So we should be good with this change.
--
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] abhisheknath2011 commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
abhisheknath2011 commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r686876290
##########
File path: gobblin-hive-registration/src/main/java/org/apache/gobblin/hive/metastore/HiveMetaStoreBasedRegister.java
##########
@@ -632,7 +632,7 @@ private void addOrAlterPartitionWithPullMode(IMetaStoreClient client, Table tabl
onPartitionExist(client, table, partition, nativePartition, existedPartition);
}
}
- } catch (TException e) {
+ } catch (NoSuchObjectException e) {
Review comment:
I think this is introduced by other three commit. Can you please change the changes specific to last two avro specific commits.
--
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] abhisheknath2011 commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
abhisheknath2011 commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r686956637
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/AvroSchemaUtils.java
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.util;
+
+import org.apache.avro.Schema;
+
+/**
+ * Avro schema utility class to perform schema property conversion to the appropriate data types
+ */
+public class AvroSchemaUtils {
+
+ private AvroSchemaUtils() {
+
+ }
+
+ public static Integer getValueAsInteger(final Schema schema, String prop) {
+ Object value = schema.getObjectProp(prop);
+ if(value instanceof Integer) {
+ return (Integer) value;
+ } else if(value instanceof String) {
Review comment:
Thanks for pointing this out. Applied Gobblin codestyle formatting.
--
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] abhisheknath2011 commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
abhisheknath2011 commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r686884929
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/AvroUtils.java
##########
@@ -147,9 +146,10 @@ public static Schema setSchemaCreationTime(Schema inputSchema, String creationTi
* Common use cases for this method is in traversing {@link Schema} object into nested level and create {@link Schema}
* object for non-root level.
*/
- public static void convertFieldToSchemaWithProps(Map<String,JsonNode> fieldProps, Schema targetSchemaObj) {
- for (Map.Entry<String, JsonNode> stringJsonNodeEntry : fieldProps.entrySet()) {
- targetSchemaObj.addProp(stringJsonNodeEntry.getKey(), stringJsonNodeEntry.getValue());
+ public static void convertFieldToSchemaWithProps(Map<String,Object> fieldProps,
Review comment:
If we keep two methods one with jsonNode and other with Object, at some point we have to clean this existing method. For example, once we upgrade to Jackson 2.x by default 1.x classes need to be removed. Need to keep the Jackson 1.x classes wherever it is absolutely necessary. For example hive-serde and helix jar has Jackson 1.x dependency. So it would be good to remove this method and it would be helpful the fix the avro changes in a cleaner way and should be able detect issues (if there any) during testing.
--
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] abhisheknath2011 commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
abhisheknath2011 commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r686865360
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -253,8 +259,16 @@ public GobblinYarnAppLauncher(Config config, YarnConfiguration yarnConfiguration
YarnHelixUtils.setYarnClassPath(config, this.yarnConfiguration);
YarnHelixUtils.setAdditionalYarnClassPath(config, this.yarnConfiguration);
this.yarnConfiguration.set("fs.automatic.close", "false");
- this.yarnClient = YarnClient.createYarnClient();
- this.yarnClient.init(this.yarnConfiguration);
+ this.originalYarnRMAddress = this.yarnConfiguration.get(GobblinYarnConfigurationKeys.YARN_RESOURCE_MANAGER_ADDRESS);
Review comment:
Please ignore this as this came from rebase. This change was introduced by one of the first three commits in the master branch. These three commits are not available in avro_1_9 and got added after rebase from upstream master.
--
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] abhisheknath2011 commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
abhisheknath2011 commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r686871949
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java
##########
@@ -37,6 +37,8 @@
public static final int DEFAULT_RELEASED_CONTAINERS_CACHE_EXPIRY_SECS = 300;
public static final String APP_VIEW_ACL = GOBBLIN_YARN_PREFIX + "appViewAcl";
public static final String DEFAULT_APP_VIEW_ACL = "*";
+ public static final String YARN_RESOURCE_MANAGER_ADDRESS = "yarn.resourcemanager.address";
Review comment:
Same as above.
--
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] autumnust commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r687910429
##########
File path: gobblin-core-base/src/main/java/org/apache/gobblin/test/SequentialTestSource.java
##########
@@ -29,7 +29,7 @@
import com.typesafe.config.Config;
import com.typesafe.config.ConfigFactory;
-import avro.shaded.com.google.common.base.Throwables;
+import com.google.common.base.Throwables;
Review comment:
Sounds good.
--
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] autumnust merged pull request #3349: [Branch avro_1_9] Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
autumnust merged pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349
--
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] autumnust commented on a change in pull request #3349: Avro 1.9 upgrade compatible change - replaced deprecated public APIs with the compatible APIs
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3349:
URL: https://github.com/apache/gobblin/pull/3349#discussion_r686476501
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -253,8 +259,16 @@ public GobblinYarnAppLauncher(Config config, YarnConfiguration yarnConfiguration
YarnHelixUtils.setYarnClassPath(config, this.yarnConfiguration);
YarnHelixUtils.setAdditionalYarnClassPath(config, this.yarnConfiguration);
this.yarnConfiguration.set("fs.automatic.close", "false");
- this.yarnClient = YarnClient.createYarnClient();
- this.yarnClient.init(this.yarnConfiguration);
+ this.originalYarnRMAddress = this.yarnConfiguration.get(GobblinYarnConfigurationKeys.YARN_RESOURCE_MANAGER_ADDRESS);
Review comment:
this doesn't seem to be relevant to your PR. Is this accidentally being brought in ? Can you rebase from the upstream if that's the case?
##########
File path: gobblin-hive-registration/src/main/java/org/apache/gobblin/hive/metastore/HiveMetaStoreBasedRegister.java
##########
@@ -632,7 +632,7 @@ private void addOrAlterPartitionWithPullMode(IMetaStoreClient client, Table tabl
onPartitionExist(client, table, partition, nativePartition, existedPartition);
}
}
- } catch (TException e) {
+ } catch (NoSuchObjectException e) {
Review comment:
is this relevant ?
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java
##########
@@ -37,6 +37,8 @@
public static final int DEFAULT_RELEASED_CONTAINERS_CACHE_EXPIRY_SECS = 300;
public static final String APP_VIEW_ACL = GOBBLIN_YARN_PREFIX + "appViewAcl";
public static final String DEFAULT_APP_VIEW_ACL = "*";
+ public static final String YARN_RESOURCE_MANAGER_ADDRESS = "yarn.resourcemanager.address";
Review comment:
same as above comments about relevance.
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/AvroSchemaUtils.java
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.util;
+
+import org.apache.avro.Schema;
+
+/**
+ * Avro schema utility class to perform schema property conversion to the appropriate data types
+ */
+public class AvroSchemaUtils {
+
+ private AvroSchemaUtils() {
+
+ }
+
+ public static Integer getValueAsInteger(final Schema schema, String prop) {
+ Object value = schema.getObjectProp(prop);
+ if(value instanceof Integer) {
+ return (Integer) value;
+ } else if(value instanceof String) {
Review comment:
space between `(` ?
BTW you could auto-format the whole thing once you have style sheet set up in your IDE: https://gobblin.readthedocs.io/en/latest/developer-guide/CodingStyle/
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/AvroUtils.java
##########
@@ -147,9 +146,10 @@ public static Schema setSchemaCreationTime(Schema inputSchema, String creationTi
* Common use cases for this method is in traversing {@link Schema} object into nested level and create {@link Schema}
* object for non-root level.
*/
- public static void convertFieldToSchemaWithProps(Map<String,JsonNode> fieldProps, Schema targetSchemaObj) {
- for (Map.Entry<String, JsonNode> stringJsonNodeEntry : fieldProps.entrySet()) {
- targetSchemaObj.addProp(stringJsonNodeEntry.getKey(), stringJsonNodeEntry.getValue());
+ public static void convertFieldToSchemaWithProps(Map<String,Object> fieldProps,
Review comment:
Shall we preserve the original method still since it is a public static method ?
##########
File path: gobblin-core-base/src/main/java/org/apache/gobblin/test/SequentialTestSource.java
##########
@@ -29,7 +29,7 @@
import com.typesafe.config.Config;
import com.typesafe.config.ConfigFactory;
-import avro.shaded.com.google.common.base.Throwables;
+import com.google.common.base.Throwables;
Review comment:
are you sure this change is backward compatible ?
--
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