[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

[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