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 2015/09/22 03:25:28 UTC
Review Request 38588: SQOOP-2560: Sqoop2: Jetty: Create new class for
Jetty server
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38588/
-----------------------------------------------------------
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
Create new class for Jetty server
Diffs
-----
server/src/main/java/org/apache/sqoop/server/SqoopJettyConstants.java PRE-CREATION
server/src/main/java/org/apache/sqoop/server/SqoopJettyContext.java PRE-CREATION
server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java PRE-CREATION
Diff: https://reviews.apache.org/r/38588/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 38588: SQOOP-2560: Sqoop2: Jetty: Create new class
for Jetty server
Posted by Colin Ma <ju...@intel.com>.
> On Sept. 22, 2015, 3:01 p.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java, lines 126-129
> > <https://reviews.apache.org/r/38588/diff/1/?file=1079598#file1079598line126>
> >
> > Do we need this main?
Yes, this main is called from sqoop.sh and start the Jetty server.
> On Sept. 22, 2015, 3:01 p.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java, lines 98-106
> > <https://reviews.apache.org/r/38588/diff/1/?file=1079598#file1079598line98>
> >
> > Thinking out-lound here - would it make sense for teste purpose to use the usual "startServer()" method just started in separate thread?
> >
> > I'm thinking about that as I'm concerned that we will eventually diverge in implementation between startServer() and startServerForTest() so I'm thinking if we can somehow use exactly the same code base.
You're right, I'll update the implementation to use only one method to start Jetty for both test and production environment.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38588/#review99989
-----------------------------------------------------------
On Sept. 22, 2015, 1:25 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38588/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 1:25 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Create new class for Jetty server
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/server/SqoopJettyConstants.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyContext.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38588/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 38588: SQOOP-2560: Sqoop2: Jetty: Create new class
for Jetty server
Posted by Jarek Cecho <ja...@apache.org>.
> On Sept. 22, 2015, 3:01 p.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java, lines 98-106
> > <https://reviews.apache.org/r/38588/diff/1/?file=1079598#file1079598line98>
> >
> > Thinking out-lound here - would it make sense for teste purpose to use the usual "startServer()" method just started in separate thread?
> >
> > I'm thinking about that as I'm concerned that we will eventually diverge in implementation between startServer() and startServerForTest() so I'm thinking if we can somehow use exactly the same code base.
>
> Colin Ma wrote:
> You're right, I'll update the implementation to use only one method to start Jetty for both test and production environment.
Awesome, thanks!
> On Sept. 22, 2015, 3:01 p.m., Jarek Cecho wrote:
> > server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java, lines 126-129
> > <https://reviews.apache.org/r/38588/diff/1/?file=1079598#file1079598line126>
> >
> > Do we need this main?
>
> Colin Ma wrote:
> Yes, this main is called from sqoop.sh and start the Jetty server.
Yeah I've realized that when I got to the other subtasks. Make sense to me!
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38588/#review99989
-----------------------------------------------------------
On Sept. 23, 2015, 7:30 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38588/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 7:30 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Create new class for Jetty server
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/server/SqoopJettyConstants.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyContext.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38588/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 38588: SQOOP-2560: Sqoop2: Jetty: Create new class
for Jetty server
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38588/#review99989
-----------------------------------------------------------
server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java (lines 98 - 106)
<https://reviews.apache.org/r/38588/#comment157053>
Thinking out-lound here - would it make sense for teste purpose to use the usual "startServer()" method just started in separate thread?
I'm thinking about that as I'm concerned that we will eventually diverge in implementation between startServer() and startServerForTest() so I'm thinking if we can somehow use exactly the same code base.
server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java (lines 126 - 129)
<https://reviews.apache.org/r/38588/#comment157052>
Do we need this main?
- Jarek Cecho
On Sept. 22, 2015, 1:25 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38588/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 1:25 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Create new class for Jetty server
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/server/SqoopJettyConstants.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyContext.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38588/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 38588: SQOOP-2560: Sqoop2: Jetty: Create new class
for Jetty server
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38588/#review100468
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On Sept. 23, 2015, 7:30 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38588/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 7:30 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Create new class for Jetty server
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/server/SqoopJettyConstants.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyContext.java PRE-CREATION
> server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38588/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 38588: SQOOP-2560: Sqoop2: Jetty: Create new class
for Jetty server
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38588/
-----------------------------------------------------------
(Updated Sept. 23, 2015, 7:30 a.m.)
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
Create new class for Jetty server
Diffs (updated)
-----
server/src/main/java/org/apache/sqoop/server/SqoopJettyConstants.java PRE-CREATION
server/src/main/java/org/apache/sqoop/server/SqoopJettyContext.java PRE-CREATION
server/src/main/java/org/apache/sqoop/server/SqoopJettyServer.java PRE-CREATION
Diff: https://reviews.apache.org/r/38588/diff/
Testing
-------
Thanks,
Colin Ma