----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11920/#review22027 -----------------------------------------------------------
Hi Mengwei, thank you very much for working on this patch, greatly appreciated! core/src/main/java/org/apache/sqoop/core/Reconfigurable.java <https://reviews.apache.org/r/11920/#comment45346> Can we provide Java doc describing purpose of the interface and the method itself? core/src/main/java/org/apache/sqoop/core/Reconfigurable.java <https://reviews.apache.org/r/11920/#comment45347> I would advise to rename this method to be more informative about what has happened, for example configurationChanged() or something similar. core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/11920/#comment45348> I would advise that each particular component would register the listener callback when the component seems fit. Registering each manager inside the SqoopConfiguration might be quite dangerous as it might happen that the reconfiguration will be called before the Manager initialization method which can be very tricky. core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/11920/#comment45349> I think that this copying of the configuration Map is artifact from previous code. Current "MapContext" implementation makes sure that the underlying map won't get changed, so I would recommend to simply return "new MapContext(oldConfig)". This can be also applied to line 191 in method getContext(). core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java <https://reviews.apache.org/r/11920/#comment45350> I would advise for each component to do it's own logging. When all logging is done by one logger, we will loose severity (all lines are marked as info) and also it will be hard to get originating class. core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java <https://reviews.apache.org/r/11920/#comment45405> There seems to be duplicity with similar code fragment in initialize() method, maybe it's worth abstracting into a method? core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java <https://reviews.apache.org/r/11920/#comment45355> I would recommend to also interrupt the purgeThread and updateThread in order to immediately apply the changes. Jarcec - Jarek Cecho On June 17, 2013, 10:33 p.m., Mengwei Ding wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11920/ > ----------------------------------------------------------- > > (Updated June 17, 2013, 10:33 p.m.) > > > Review request for Sqoop, Jarek Cecho, Hari Shreedharan, and Abraham Elmahrek. > > > Description > ------- > > commit 719aeaab2cd74338399f43c411f7dccd95089c7b > Author: Mengwei Ding <[email protected]> > Date: Wed Jun 12 15:47:26 2013 -0700 > > SQOOP-971 Add dynamic reconfiguration ability to RepositoryManager, > ConnectorManager and FrameworkManager > > :100644 100644 500189a... 5ea1ab3... M > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java > :100644 100644 f59d132... eb7c1dc... M > core/src/main/java/org/apache/sqoop/core/CoreError.java > :000000 100644 0000000... 3c7d004... A > core/src/main/java/org/apache/sqoop/core/Reconfigurable.java > :100644 100644 deb24c9... 6d9a177... M > core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java > :100644 100644 145a2c1... 38fa932... M > core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java > :100644 100644 3339c59... 6240aac... M > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryProvider.java > :100644 100644 955306d... 53aef5d... M > core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java > :100644 100644 4ea52e9... 1ec6bdf... M > core/src/main/java/org/apache/sqoop/repository/RepositoryProvider.java > :100644 100644 de9a24b... ae8686b... M > server/src/main/java/org/apache/sqoop/handler/SubmissionRequestHandler.java > > > This addresses bug SQOOP-971. > https://issues.apache.org/jira/browse/SQOOP-971 > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 500189a > core/src/main/java/org/apache/sqoop/core/CoreError.java f59d132 > core/src/main/java/org/apache/sqoop/core/Reconfigurable.java PRE-CREATION > core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java deb24c9 > core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java 145a2c1 > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryProvider.java > 3339c59 > core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java > 955306d > core/src/main/java/org/apache/sqoop/repository/RepositoryProvider.java > 4ea52e9 > server/src/main/java/org/apache/sqoop/handler/SubmissionRequestHandler.java > de9a24b > > Diff: https://reviews.apache.org/r/11920/diff/ > > > Testing > ------- > > > Thanks, > > Mengwei Ding > >
