You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2020/09/26 15:58:59 UTC
[GitHub] [hadoop] maobaolong opened a new pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
maobaolong opened a new pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341
HADOOP-17287.
This PR provide possibility to new instance by non default constructor through ReflectionUtils
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#issuecomment-705904691
@sunchao Thanks for your review and merge this PR.
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#issuecomment-699575054
@sunchao Thanks for your review, I add a new test and fix the typo issues.
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on a change in pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on a change in pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#discussion_r497248808
##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestReflectionUtils.java
##########
@@ -168,6 +168,13 @@ public void testLogThreadInfo() throws Exception {
containsString("Process Thread Dump: " + title));
}
+ @Test
+ public void testNewInstanceForNonDefaultConstructor() {
+ Object x = ReflectionUtils.newInstance(
+ NoDefaultCtor.class, null, new Class[] {int.class}, 1);
+ Assert.assertTrue(x instanceof NoDefaultCtor);
Review comment:
Thanks for check this for me, done
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#issuecomment-705904691
@sunchao Thanks for your review and merge this PR.
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on a change in pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on a change in pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#discussion_r497248700
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java
##########
@@ -124,15 +124,29 @@ private static void setJobConf(Object theObject, Configuration conf) {
*/
@SuppressWarnings("unchecked")
public static <T> T newInstance(Class<T> theClass, Configuration conf) {
+ return newInstance(theClass, conf, EMPTY_ARRAY);
+ }
+
+ /** Create an object for the given class and initialize it from conf
+ *
+ * @param theClass class of which an object is created
+ * @param conf Configuration
+ * @param argTypes the types of the arguments
+ * @param values the values of the arguments
+ * @return a new object
+ */
+ @SuppressWarnings("unchecked")
+ public static <T> T newInstance(Class<T> theClass, Configuration conf,
+ Class<?>[] argTypes, Object ... values) {
Review comment:
Good idea, done.
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] sunchao merged pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
sunchao merged pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] sunchao commented on a change in pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#discussion_r496095888
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java
##########
@@ -124,15 +124,29 @@ private static void setJobConf(Object theObject, Configuration conf) {
*/
@SuppressWarnings("unchecked")
public static <T> T newInstance(Class<T> theClass, Configuration conf) {
+ return newInstance(theClass, conf, EMPTY_ARRAY);
+ }
+
+ /** Create an object for the given class and initialize it from conf
+ *
+ * @param theClass class of which an object is created
+ * @param conf Configuration
+ * @param argTypes the types of the arguments
+ * @param values the values of the arguments
+ * @return a new object
+ */
+ @SuppressWarnings("unchecked")
+ public static <T> T newInstance(Class<T> theClass, Configuration conf,
+ Class<?>[] argTypes, Object ... values) {
Review comment:
One more thing we can do here is perhaps checking whether `argTypes.length == values.length`, and throw `IllegalArgumentException` like: "1 parameters are required but 0 arguments are provided". This will be more informative than the default exception message.
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#issuecomment-701162354
@sunchao Thanks for the review and point a lost import for me. Now I add an argument num check and a new test case.
And, I test the two tests which added by me, both are passed.
Please take another look, thank you.
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on a change in pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on a change in pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#discussion_r495583920
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java
##########
@@ -124,15 +124,29 @@ private static void setJobConf(Object theObject, Configuration conf) {
*/
@SuppressWarnings("unchecked")
public static <T> T newInstance(Class<T> theClass, Configuration conf) {
+ return newInstance(theClass, conf, EMPTY_ARRAY);
+ }
+
+ /** Create an object for the given class and initialize it from conf
+ *
+ * @param theClass class of which an object is created
+ * @param conf Configuration
+ * @param argsType the types of the arguments
+ * @param values the values of the arguments
+ * @return a new object
+ */
+ @SuppressWarnings("unchecked")
+ public static <T> T newInstance(Class<T> theClass, Configuration conf,
Review comment:
Done
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] sunchao commented on a change in pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#discussion_r496095888
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java
##########
@@ -124,15 +124,29 @@ private static void setJobConf(Object theObject, Configuration conf) {
*/
@SuppressWarnings("unchecked")
public static <T> T newInstance(Class<T> theClass, Configuration conf) {
+ return newInstance(theClass, conf, EMPTY_ARRAY);
+ }
+
+ /** Create an object for the given class and initialize it from conf
+ *
+ * @param theClass class of which an object is created
+ * @param conf Configuration
+ * @param argTypes the types of the arguments
+ * @param values the values of the arguments
+ * @return a new object
+ */
+ @SuppressWarnings("unchecked")
+ public static <T> T newInstance(Class<T> theClass, Configuration conf,
+ Class<?>[] argTypes, Object ... values) {
Review comment:
One more thing we can do here is perhaps checking whether `argTypes.length == values.length`, and throw `IllegalArgumentException` like: "1 parameters are required but 0 arguments are provided".
##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestReflectionUtils.java
##########
@@ -168,6 +168,13 @@ public void testLogThreadInfo() throws Exception {
containsString("Process Thread Dump: " + title));
}
+ @Test
+ public void testNewInstanceForNonDefaultConstructor() {
+ Object x = ReflectionUtils.newInstance(
+ NoDefaultCtor.class, null, new Class[] {int.class}, 1);
+ Assert.assertTrue(x instanceof NoDefaultCtor);
Review comment:
I think `Assert` is not imported and compilation will fail. Also can we test the cases when number of parameter and argument doesn't match?
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] sunchao commented on a change in pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#discussion_r495478153
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java
##########
@@ -124,15 +124,29 @@ private static void setJobConf(Object theObject, Configuration conf) {
*/
@SuppressWarnings("unchecked")
public static <T> T newInstance(Class<T> theClass, Configuration conf) {
+ return newInstance(theClass, conf, EMPTY_ARRAY);
+ }
+
+ /** Create an object for the given class and initialize it from conf
+ *
+ * @param theClass class of which an object is created
+ * @param conf Configuration
+ * @param argsType the types of the arguments
+ * @param values the values of the arguments
+ * @return a new object
+ */
+ @SuppressWarnings("unchecked")
+ public static <T> T newInstance(Class<T> theClass, Configuration conf,
Review comment:
Could you add a test in `TestReflectionUtils`?
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java
##########
@@ -124,15 +124,29 @@ private static void setJobConf(Object theObject, Configuration conf) {
*/
@SuppressWarnings("unchecked")
public static <T> T newInstance(Class<T> theClass, Configuration conf) {
+ return newInstance(theClass, conf, EMPTY_ARRAY);
+ }
+
+ /** Create an object for the given class and initialize it from conf
+ *
+ * @param theClass class of which an object is created
+ * @param conf Configuration
+ * @param argsType the types of the arguments
+ * @param values the values of the arguments
+ * @return a new object
+ */
+ @SuppressWarnings("unchecked")
+ public static <T> T newInstance(Class<T> theClass, Configuration conf,
+ Class<?>[] argsType, Object ... values) {
Review comment:
nit: `argsType` -> `argTypes`
----------------------------------------------------------------
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:
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
[GitHub] [hadoop] maobaolong commented on a change in pull request #2341: HADOOP-17287. Support new Instance by non default constructor by ReflectionUtils
Posted by GitBox <gi...@apache.org>.
maobaolong commented on a change in pull request #2341:
URL: https://github.com/apache/hadoop/pull/2341#discussion_r495583929
##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java
##########
@@ -124,15 +124,29 @@ private static void setJobConf(Object theObject, Configuration conf) {
*/
@SuppressWarnings("unchecked")
public static <T> T newInstance(Class<T> theClass, Configuration conf) {
+ return newInstance(theClass, conf, EMPTY_ARRAY);
+ }
+
+ /** Create an object for the given class and initialize it from conf
+ *
+ * @param theClass class of which an object is created
+ * @param conf Configuration
+ * @param argsType the types of the arguments
+ * @param values the values of the arguments
+ * @return a new object
+ */
+ @SuppressWarnings("unchecked")
+ public static <T> T newInstance(Class<T> theClass, Configuration conf,
+ Class<?>[] argsType, Object ... values) {
Review comment:
Done
----------------------------------------------------------------
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:
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