You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Dian Fu <di...@gmail.com> on 2015/09/23 12:56:27 UTC
Review Request 38670: Sqoop2: Support loading classes using custom
classloader in ClassUtils
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38670/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2577
https://issues.apache.org/jira/browse/SQOOP-2577
Repository: sqoop-sqoop2
Description
-------
Currently, ClassUtils doesn't support to load classes using a custom classloader. The aim of this JIRA is to provide ClassUtils with this capacity.
Diffs
-----
common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802
Diff: https://reviews.apache.org/r/38670/diff/
Testing
-------
Thanks,
Dian Fu
Re: Review Request 38670: Sqoop2: Support loading classes using custom
classloader in ClassUtils
Posted by Dian Fu <di...@gmail.com>.
> On Sept. 24, 2015, 8:50 p.m., Jarek Cecho wrote:
> > Overall the code looks good to me, but I have few questions:
> >
> > 1) Since this is significant change, we should provide some tests for it. I'm not immediately sure how to test those though. Any thoughts Dian?
> >
> > 2) I was wondering what are we gainging by caching the loaded classes? Is it faster then without the caching?
> >
> > 3) I didn't fully understood the need for NEGATIVE_CACHE_SENTINEL - can't be "null" used instead of it?
> >
> > Jarcec
Thanks Jarcec for the review.
1) For the tests, I have updated the patch adding one test case. But this test case has dependency to SQOOP-2578, so I have canceled the patch available state of this JIRA and will set it back when SQOOP-2578 is in.
2) For caching the classes and NEGATIVE_CACHE_SENTINEL, in fact I borrowed that implementation from Hadoop. Caching will be faster(refer to HADOOP-6133 for details), but I think that class loading time won't be a problem for SQOOP 2 and so I'm ok to remove the cache. But on the other side, keeping it there should have no harm. :)
3) If you agree to keep cache there, then NEGATIVE_CACHE_SENTINEL will be necessary. (HADOOP-8157 has detailed discussion on this)
- Dian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38670/#review100461
-----------------------------------------------------------
On Sept. 25, 2015, 5:41 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38670/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2015, 5:41 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2577
> https://issues.apache.org/jira/browse/SQOOP-2577
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, ClassUtils doesn't support to load classes using a custom classloader. The aim of this JIRA is to provide ClassUtils with this capacity.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802
> common/src/test/java/org/apache/sqoop/utils/TestClassUtils.java 4126e7b
>
> Diff: https://reviews.apache.org/r/38670/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 38670: Sqoop2: Support loading classes using custom
classloader in ClassUtils
Posted by Jarek Cecho <ja...@apache.org>.
> On Sept. 24, 2015, 8:50 p.m., Jarek Cecho wrote:
> > Overall the code looks good to me, but I have few questions:
> >
> > 1) Since this is significant change, we should provide some tests for it. I'm not immediately sure how to test those though. Any thoughts Dian?
> >
> > 2) I was wondering what are we gainging by caching the loaded classes? Is it faster then without the caching?
> >
> > 3) I didn't fully understood the need for NEGATIVE_CACHE_SENTINEL - can't be "null" used instead of it?
> >
> > Jarcec
>
> Dian Fu wrote:
> Thanks Jarcec for the review.
> 1) For the tests, I have updated the patch adding one test case. But this test case has dependency to SQOOP-2578, so I have canceled the patch available state of this JIRA and will set it back when SQOOP-2578 is in.
> 2) For caching the classes and NEGATIVE_CACHE_SENTINEL, in fact I borrowed that implementation from Hadoop. Caching will be faster(refer to HADOOP-6133 for details), but I think that class loading time won't be a problem for SQOOP 2 and so I'm ok to remove the cache. But on the other side, keeping it there should have no harm. :)
> 3) If you agree to keep cache there, then NEGATIVE_CACHE_SENTINEL will be necessary. (HADOOP-8157 has detailed discussion on this)
Thanks for all the pointers Dian! It was helpful for me to go through the comments to understand the reasoning behind those.
1) Thanks for the rudimentary test. It's not covering all the options, but I think that the rest of the code will excercise it appropriately. Hence I'm fine with this.
2) + 3) The pointers to Hadoop JIRAs are extremely helpful. Consider my points as moot :)
I'll finish the review and hopefully commit this soon.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38670/#review100461
-----------------------------------------------------------
On Sept. 25, 2015, 5:41 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38670/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2015, 5:41 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2577
> https://issues.apache.org/jira/browse/SQOOP-2577
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, ClassUtils doesn't support to load classes using a custom classloader. The aim of this JIRA is to provide ClassUtils with this capacity.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802
> common/src/test/java/org/apache/sqoop/utils/TestClassUtils.java 4126e7b
>
> Diff: https://reviews.apache.org/r/38670/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 38670: Sqoop2: Support loading classes using custom
classloader in ClassUtils
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38670/#review100461
-----------------------------------------------------------
Overall the code looks good to me, but I have few questions:
1) Since this is significant change, we should provide some tests for it. I'm not immediately sure how to test those though. Any thoughts Dian?
2) I was wondering what are we gainging by caching the loaded classes? Is it faster then without the caching?
3) I didn't fully understood the need for NEGATIVE_CACHE_SENTINEL - can't be "null" used instead of it?
Jarcec
- Jarek Cecho
On Sept. 24, 2015, 1:17 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38670/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 1:17 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2577
> https://issues.apache.org/jira/browse/SQOOP-2577
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, ClassUtils doesn't support to load classes using a custom classloader. The aim of this JIRA is to provide ClassUtils with this capacity.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802
>
> Diff: https://reviews.apache.org/r/38670/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 38670: Sqoop2: Support loading classes using custom
classloader in ClassUtils
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38670/#review100836
-----------------------------------------------------------
Ship it!
+1 as long as precommit hook passes
- Jarek Cecho
On Sept. 25, 2015, 5:41 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38670/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2015, 5:41 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2577
> https://issues.apache.org/jira/browse/SQOOP-2577
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, ClassUtils doesn't support to load classes using a custom classloader. The aim of this JIRA is to provide ClassUtils with this capacity.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802
> common/src/test/java/org/apache/sqoop/utils/TestClassUtils.java 4126e7b
>
> Diff: https://reviews.apache.org/r/38670/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 38670: Sqoop2: Support loading classes using custom
classloader in ClassUtils
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38670/
-----------------------------------------------------------
(Updated Sept. 25, 2015, 5:41 a.m.)
Review request for Sqoop.
Bugs: SQOOP-2577
https://issues.apache.org/jira/browse/SQOOP-2577
Repository: sqoop-sqoop2
Description
-------
Currently, ClassUtils doesn't support to load classes using a custom classloader. The aim of this JIRA is to provide ClassUtils with this capacity.
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802
common/src/test/java/org/apache/sqoop/utils/TestClassUtils.java 4126e7b
Diff: https://reviews.apache.org/r/38670/diff/
Testing
-------
Thanks,
Dian Fu
Re: Review Request 38670: Sqoop2: Support loading classes using custom
classloader in ClassUtils
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38670/
-----------------------------------------------------------
(Updated Sept. 24, 2015, 1:17 a.m.)
Review request for Sqoop.
Bugs: SQOOP-2577
https://issues.apache.org/jira/browse/SQOOP-2577
Repository: sqoop-sqoop2
Description
-------
Currently, ClassUtils doesn't support to load classes using a custom classloader. The aim of this JIRA is to provide ClassUtils with this capacity.
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 6262802
Diff: https://reviews.apache.org/r/38670/diff/
Testing
-------
Thanks,
Dian Fu