sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Veena Basavaraj" <vbasava...@cloudera.com>
Subject Re: Review Request 27330: SQOOP-1510:JobRequestHandler for submit/abort job and SubmissionHandler changes
Date Fri, 31 Oct 2014 14:53:56 GMT


> On Oct. 31, 2014, 6:57 a.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/SqoopClient.java, line 85
> > <https://reviews.apache.org/r/27330/diff/7/?file=743043#file743043line85>
> >
> >     I do feel that "SUBMITTED" and "STARTED" are two different cases. Submitted
means that we've created the request to start a new job that will eventually transfer data.
Started means that the job has started and is running now.
> >     
> >     This call will only create that request and will return before the actuall job
will START. I don't see much different on our testing cluster as Submitted job is started
pretty much immediately, however on real clusters the delay between submission and start can
be significant as the job can be sitting in a scheduler queue for substantial amount of time
before it starts (especially on busy clusters).

there are subtle differences, submit and start are used in very different ways. 

Think about this from the time we issue a command in the client, going by your explanation
the command should be submit. I am utterly confused reading this code and how we have just
used started and submitted without much thought.

I appreciate if we look at this from the client command / status to the execution engine on
what we want to call as submit and what should be start.


> On Oct. 31, 2014, 6:57 a.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/json/JobsBean.java, line 29
> > <https://reviews.apache.org/r/27330/diff/7/?file=743049#file743049line29>
> >
> >     I'm wondering why we are introducing this plural bean? I've noticed that we
did something similar for the link recently as well.
> >     
> >     All our beans has been historically written in a way that they supported 1..N
objects. Hence they were usable in all cases. This seems unnecessary change to me as now the
code using the beans needs to have if-else statements to distinguish what exactly is being
send.

read the JIRA and the related tickets for 1509, they are two different JSON structures, historically
done does not mean it has to stay that unless it is making it obvious.

if I sending a JOB, I would see the root to be JOB, if I request a collection it better be
JOBS.


> On Oct. 31, 2014, 6:57 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java, lines 508-520
> > <https://reviews.apache.org/r/27330/diff/7/?file=743062#file743062line508>
> >
> >     This move seems unencessary?

why? would you care to ask why I felt it is necessary? 

when reading code, having all the links related apis in one places makes it wasy for some
one to not miss the apis. Whats wrong with moving, you want another RB for it? say that. 

Jus think about how you organize your table, when we write code lets do the same, organize
things so that at one shot we know what the apis for links are. I would even want to split
this up into logical classes as we have groen the repository apis, see how hard it is to add
post gres, wish we had thight through this before.


> On Oct. 31, 2014, 6:57 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java, line
360
> > <https://reviews.apache.org/r/27330/diff/7/?file=743063#file743063line360>
> >
> >     Repository can't submit job as that is a job of execution engine. Repository
is just storing the submission metadata. Hence I would prefer to keep the original name as
this one seems misleading to me.

this has been changed, see the JIRA, this RB is stale .


> On Oct. 31, 2014, 6:57 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java, line
392
> > <https://reviews.apache.org/r/27330/diff/7/?file=743063#file743063line392>
> >
> >     Just FYI: Entire code base is written in a way that all methods, variables,
properties are from most generic to most specific. So that when you sort all the methods alphabetically
(which majority of the tools does), you can see all methods for let say "submission" next
to each other. This entire review is changing that on majority of the places to "common english".

> >     
> >     I'm wondering if there is a technical reason for that?

it is not, why do we call it findSbmissions and not Submissions find, why to we call it updateLink
than linkUpdate? entire is a strong word

How does find become the prefix and not Unfinished.


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27330/#review59331
-----------------------------------------------------------


On Oct. 30, 2014, 2:08 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27330/
> -----------------------------------------------------------
> 
> (Updated Oct. 30, 2014, 2:08 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see JIRA and its parent JIRA for details
> 
> All the WS stuff, I will do it in the end once the functionality is reviewd.
> 
> 
> If someone is wondering why START is changed to submit: there are tons of places in the
code and in the java docs, we actually mean submit when we say START
> 
>   DRIVER_0008("Invalid combination of submission and execution engines"),
> 
>   DRIVER_0009("Job has been disabled. Cannot submit this job."),
> 
>   DRIVER_0010("Link for this job has been disabled. Cannot submit this job."),
> 
>   DRIVER_0011("Connector does not support specified direction. Cannot submit this job."),
> 
>   ;
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/sqoop/client/SqoopClient.java 33a0c3c 
>   client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java de7211a 
>   client/src/main/java/org/apache/sqoop/client/request/JobResourceRequest.java 83c08b3

>   client/src/main/java/org/apache/sqoop/client/request/SqoopResourceRequests.java 4a56bb7

>   client/src/main/java/org/apache/sqoop/client/request/SubmissionResourceRequest.java
5055783 
>   common/src/main/java/org/apache/sqoop/json/JobBean.java 082d591 
>   common/src/main/java/org/apache/sqoop/json/JobsBean.java PRE-CREATION 
>   common/src/main/java/org/apache/sqoop/json/JsonBean.java ba86511 
>   common/src/main/java/org/apache/sqoop/json/LinksBean.java 5858a18 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 4b80338 
>   common/src/main/java/org/apache/sqoop/json/SubmissionsBean.java PRE-CREATION 
>   common/src/main/java/org/apache/sqoop/model/MLink.java 7a9f538 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 7290df5 
>   common/src/test/java/org/apache/sqoop/json/TestJobBean.java 1fc8dbd 
>   common/src/test/java/org/apache/sqoop/json/TestLinkBean.java 811cbf0 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java e4d50bf 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java
2411169 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java
3ae64f0 
>   core/src/main/java/org/apache/sqoop/driver/JobManager.java f6447c6 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 2aeb07e 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java ad380d3 
>   core/src/main/java/org/apache/sqoop/repository/Repository.java 79742b9 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
b996a0b 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaInsertUpdateDeleteSelectQuery.java
c894d06 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java
4c2d062 
>   server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java 5694ea5

>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 5547988 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 35a9635 
>   server/src/main/java/org/apache/sqoop/handler/SubmissionRequestHandler.java 8555b0c

>   server/src/main/java/org/apache/sqoop/server/RequestHandler.java 508edd2 
>   server/src/main/java/org/apache/sqoop/server/v1/JobServlet.java d295237 
>   server/src/main/java/org/apache/sqoop/server/v1/JobsServlet.java PRE-CREATION 
>   server/src/main/java/org/apache/sqoop/server/v1/LinkServlet.java 127903a 
>   server/src/main/java/org/apache/sqoop/server/v1/SubmissionServlet.java 5c1d883 
>   server/src/main/java/org/apache/sqoop/server/v1/SubmissionsServlet.java PRE-CREATION

>   server/src/main/webapp/WEB-INF/web.xml 6ad90d2 
>   shell/src/main/java/org/apache/sqoop/shell/AbortCommand.java PRE-CREATION 
>   shell/src/main/java/org/apache/sqoop/shell/ShowJobStatusFunction.java PRE-CREATION

>   shell/src/main/java/org/apache/sqoop/shell/SqoopShell.java 2e87965 
>   shell/src/main/java/org/apache/sqoop/shell/StartCommand.java 7c56980 
>   shell/src/main/java/org/apache/sqoop/shell/StartJobFunction.java 4363f05 
>   shell/src/main/java/org/apache/sqoop/shell/StatusCommand.java 3447a87 
>   shell/src/main/java/org/apache/sqoop/shell/StatusJobFunction.java fb83af3 
>   shell/src/main/java/org/apache/sqoop/shell/StopCommand.java 50b2e81 
>   shell/src/main/java/org/apache/sqoop/shell/StopJobFunction.java 790c522 
>   shell/src/main/java/org/apache/sqoop/shell/SubmitCommand.java PRE-CREATION 
>   shell/src/main/java/org/apache/sqoop/shell/SubmitJobFunction.java PRE-CREATION 
>   shell/src/main/java/org/apache/sqoop/shell/UpdateJobFunction.java dd075d7 
>   shell/src/main/java/org/apache/sqoop/shell/UpdateLinkFunction.java 176833a 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java 44d5920 
>   shell/src/main/resources/shell-resource.properties 0e63c50 
>   test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java af31769 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java
36f7443 
>   tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryDumpTool.java c219e68 
>   tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java 64b08fc 
> 
> Diff: https://reviews.apache.org/r/27330/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message