[GitHub] [hadoop] omalley commented on a diff in pull request #4215: HADOOP-18215. Enhance WritableName to be able to return aliases for classes that use serializers

["omalley (via GitHub)" <gi...@apache.org>]

omalley commented on code in PR #4215:
URL: https://github.com/apache/hadoop/pull/4215#discussion_r1107882426


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableName.java:
##########
@@ -79,20 +79,42 @@ public static synchronized String getName(Class<?> writableClass) {
     return writableClass.getName();
   }
 
+  /**
+   * Return the class for a name. Requires the class for name to extend Writable.
+   * See {@link #getClass(String, Configuration, boolean)} if class doesn't extend Writable.
+   * Default is {@link Class#forName(String)}.
+   *
+   * @param name input name.
+   * @param conf input configuration.
+   * @return class for a name.
+   * @throws IOException raised on errors performing I/O.
+   */
+  public static synchronized Class<?> getClass(String name, Configuration conf)
+      throws IOException {
+    return getClass(name, conf, true);
+  }
+
   /**
    * Return the class for a name.
    * Default is {@link Class#forName(String)}.
    *
    * @param name input name.
    * @param conf input configuration.
+   * @param requireWritable if true, require the class for name to extend Writable
    * @return class for a name.
    * @throws IOException raised on errors performing I/O.
    */
-  public static synchronized Class<?> getClass(String name, Configuration conf
-                                            ) throws IOException {
+  public static synchronized Class<?> getClass(String name, Configuration conf,
+      boolean requireWritable) throws IOException {

Review Comment:
   I'd propose that we unconditionally remove the check for Writable in getClass. My thought is:
   * Users can always enforce the constraint later if they want to.
   * All uses of the method with Hadoop's code base don't want to limit the output.
   * The check isn't consistent. (It is only applied for aliases, not natural class names.)
   * Removing the check won't break any callers since they couldn't get non-Writables before.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org