> On Jan. 19, 2016, 6:37 p.m., Abraham Fine wrote: > > common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.java, line 45 > > <https://reviews.apache.org/r/42446/diff/2/?file=1199712#file1199712line45> > > > > not sure what this comment means
Thanks for catch that, fixed. > On Jan. 19, 2016, 6:37 p.m., Abraham Fine wrote: > > common/src/main/java/org/apache/sqoop/utils/ReflectionUtils.java, line 30 > > <https://reviews.apache.org/r/42446/diff/2/?file=1199713#file1199713line30> > > > > are we only including this class because the ReflectionUtils class > > included in one version of hadoop results in a firebugs warning? > > > > it seems strange to me to not use a hadoop class because firebugs is > > unhappy with it. isn't hadoop a "provided" dependency anyway? > > > > perhaps it would make more sense to ignore the firebugs warning rather > > than writing a whole new class? Yes, a findbugs warning make me copy the ReflectionUtils from Hadoop 2.7.1, but you're right, using the Hadoop ReflectionUtils and ignore the findbugs warning for now. > On Jan. 19, 2016, 6:37 p.m., Abraham Fine wrote: > > server/src/main/java/org/apache/sqoop/handler/ThreadDumpRequestHandler.java, > > line 41 > > <https://reviews.apache.org/r/42446/diff/2/?file=1199714#file1199714line41> > > > > would returning an http code 405 make any sense here? After check the other implementation of RequestHandler, throw the exception SERVER_0002("Unsupported HTTP method") for this situation. > On Jan. 19, 2016, 6:37 p.m., Abraham Fine wrote: > > shell/src/main/java/org/apache/sqoop/shell/core/Constants.java, line 489 > > <https://reviews.apache.org/r/42446/diff/2/?file=1199719#file1199719line489> > > > > nitpick: make this match the indentation of the other constants This variable is not used, removed. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42446/#review115202 ----------------------------------------------------------- On Jan. 18, 2016, 5:45 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42446/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2016, 5:45 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Add web interface for thread dump, user can get the information from the > shell. > > > Diffs > ----- > > client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e > > client/src/main/java/org/apache/sqoop/client/request/SqoopResourceRequests.java > 33c90a8 > > client/src/main/java/org/apache/sqoop/client/request/ThreadDumpResourceRequest.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.java PRE-CREATION > common/src/main/java/org/apache/sqoop/utils/ReflectionUtils.java > PRE-CREATION > server/src/main/java/org/apache/sqoop/handler/ThreadDumpRequestHandler.java > PRE-CREATION > server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java 2c4cb7a > server/src/main/java/org/apache/sqoop/server/ThreadDumpServlet.java > PRE-CREATION > shell/src/main/java/org/apache/sqoop/shell/ShowCommand.java eb8522a > shell/src/main/java/org/apache/sqoop/shell/ShowThreadDumpFunction.java > PRE-CREATION > shell/src/main/java/org/apache/sqoop/shell/core/Constants.java 9c57a2e > shell/src/main/resources/shell-resource.properties 630c31d > test/src/test/java/org/apache/sqoop/integration/shell/ShowCommandTest.java > 9fd4811 > > Diff: https://reviews.apache.org/r/42446/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
