You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Colin Ma <ju...@intel.com> on 2016/01/18 03:07:43 UTC
Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42446/
-----------------------------------------------------------
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/pom.xml 7237608
common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.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
Re: Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
Posted by Abraham Fine <ab...@abrahamfine.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42446/#review115357
-----------------------------------------------------------
Ship it!
Ship It!
- Abraham Fine
On Jan. 20, 2016, 3:18 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42446/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 3:18 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/pom.xml 7237608
> common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.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
> test/src/test/java/org/apache/sqoop/integration/shell/ShowCommandTest.java 9fd4811
>
> Diff: https://reviews.apache.org/r/42446/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
Posted by Colin Ma <ju...@intel.com>.
> On Jan. 21, 2016, 9:46 a.m., Jarek Cecho wrote:
> > Thanks for the patch Colin!
> >
> > I was thinking about the approach and I have a small concern, so I wanted to run it by you (and the rest of the community). When I look at Hadoop and other components, the "debug" interface runs as a simple HTTP page(s) on completely different ports. E.g. the thread dump and other debug pages are not part of the normal public APIs. I believe that this is because by exposing the debug info on separate interface makes it easy to disable access to it (on firewall or even in component's own configuration). I'm concerned that by offering the debug info as part of our main REST interface we might be opening a security hole when users will start be concerned about leaking sensitive information. We could possibly solve that problem by using our authorization model to protect those calls, but I'm not sure if it's necessary. What do you think?
Good caught, it's very important to add authorization for the debug info. I'll update the patch for that.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42446/#review115601
-----------------------------------------------------------
On Jan. 20, 2016, 3:18 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42446/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 3:18 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/pom.xml 7237608
> common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.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
> test/src/test/java/org/apache/sqoop/integration/shell/ShowCommandTest.java 9fd4811
>
> Diff: https://reviews.apache.org/r/42446/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42446/#review115601
-----------------------------------------------------------
Thanks for the patch Colin!
I was thinking about the approach and I have a small concern, so I wanted to run it by you (and the rest of the community). When I look at Hadoop and other components, the "debug" interface runs as a simple HTTP page(s) on completely different ports. E.g. the thread dump and other debug pages are not part of the normal public APIs. I believe that this is because by exposing the debug info on separate interface makes it easy to disable access to it (on firewall or even in component's own configuration). I'm concerned that by offering the debug info as part of our main REST interface we might be opening a security hole when users will start be concerned about leaking sensitive information. We could possibly solve that problem by using our authorization model to protect those calls, but I'm not sure if it's necessary. What do you think?
- Jarek Cecho
On Jan. 20, 2016, 3:18 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42446/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 3:18 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/pom.xml 7237608
> common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.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
> test/src/test/java/org/apache/sqoop/integration/shell/ShowCommandTest.java 9fd4811
>
> Diff: https://reviews.apache.org/r/42446/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42446/
-----------------------------------------------------------
(Updated Jan. 20, 2016, 3:18 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 (updated)
-----
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/pom.xml 7237608
common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.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
test/src/test/java/org/apache/sqoop/integration/shell/ShowCommandTest.java 9fd4811
Diff: https://reviews.apache.org/r/42446/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
Posted by Colin Ma <ju...@intel.com>.
> 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
>
>
Re: Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
Posted by Abraham Fine <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42446/#review115202
-----------------------------------------------------------
common/src/main/java/org/apache/sqoop/json/ThreadDumpBean.java (line 45)
<https://reviews.apache.org/r/42446/#comment176101>
not sure what this comment means
common/src/main/java/org/apache/sqoop/utils/ReflectionUtils.java (line 28)
<https://reviews.apache.org/r/42446/#comment176105>
can be used instead of... ?
common/src/main/java/org/apache/sqoop/utils/ReflectionUtils.java (line 30)
<https://reviews.apache.org/r/42446/#comment176106>
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?
server/src/main/java/org/apache/sqoop/handler/ThreadDumpRequestHandler.java (line 41)
<https://reviews.apache.org/r/42446/#comment176104>
would returning an http code 405 make any sense here?
shell/src/main/java/org/apache/sqoop/shell/core/Constants.java (line 489)
<https://reviews.apache.org/r/42446/#comment176103>
nitpick: make this match the indentation of the other constants
test/src/test/java/org/apache/sqoop/integration/shell/ShowCommandTest.java (line 41)
<https://reviews.apache.org/r/42446/#comment176102>
why do we need this?
- Abraham Fine
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
>
>
Re: Review Request 42446: SQOOP-2776: Sqoop2: Add web interface for
thread dump
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
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 (updated)
-----
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