sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 27330: SQOOP-1510:JobRequestHandler for submit/abort job and SubmissionHandler changes
Date Mon, 03 Nov 2014 02:54:11 GMT


> On Oct. 31, 2014, 1:57 p.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).
> 
> Veena Basavaraj wrote:
>     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.
> 
> Jarek Cecho wrote:
>     I think that the client and REST API have different audiences that have different
requirements. Most of the users don't know nor want to know the diffence between Submit and
Start. I feel that the fact that those are two independent steps is pretty much an "implementation
detail" from their perspective. On the other hand the connector developper or someone who
will integrate with the REST API should know the difference and hence it's exposed there.
I believe that this is intentional. 
>     
>     The project do have parts that are meant for different audiences and I think that
it's reasonable that different audiences are presented with slightly different point of view.
Simplification for users seems as good idea whereas we should say accurate for someone who
is extending Sqoop.
> 
> Veena Basavaraj wrote:
>     correct me, I am not confused here.
>     
>     There is SqoopCLient -> so who is the audience for client. My understanding is
that anyone can use it. Not retreicted to developer. Using rest/ client is a matter of preference
is what I beleive.
>     
>     Second, I do not think different audiences need a different picture, it just makes
our coding and rationalizing what we do in sqoop code harder. 
>     
>     Second, what is "most" mean? there are differences in clearly how we use them. submit
is like putting it on a queue for it to start. As you clealry artiuclated it, it might start
immediately or be delayed. There are error messages when I use a start command,it says I cannot
submit the job, what does this indicate?
>     
>     Lets fix this. It might sound very trivial to you, but going forward it will lead
to more confusion if one another person joins this team and wants his /her way.
>     
>     The rest api uses the SubmissionStatus I suppose directly and the SQoop client uses
its own enum. Why?
> 
> Veena Basavaraj wrote:
>     see JIRA for details, taking this discussion to new a JIRA and new RB.

Thanks, make sense.


> On Oct. 31, 2014, 1:57 p.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?
> 
> Veena Basavaraj wrote:
>     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.
> 
> Jarek Cecho wrote:
>     Doing unnecessary moves is breaking "git blame" and "git cherrypick". For easier
readability we have all the methods and properties named in a way that most generic part was
at the begging and most specific one at the end. This way all the overview tools (like method
listing in IDEs) is showing similar methods next to each other.
> 
> Veena Basavaraj wrote:
>     I just gave an example.
>     
>     we call it updateLink, and not linkUpdate, what is the most generic par?
>     
>     Annd if your concern is about git blame, I will do another RB first with these changes.
I have no issues with it.
> 
> Veena Basavaraj wrote:
>     Lets try to name methods like 90% of the people in other projects name it, if we
have these special rules in sqoop, I;d like to see it documented and then we take a vote on
why we need this.
>     
>     Until then I suggest we go back to what every one else names their methods by. This
is something we should start right now helping contributors to be not confused on how to name
a method and have a coherent story.
>     
>     First thing after this RB is shipped I will start the coding standard wiki and cover
these examples

I'm all for specifying public code guidelines, let's create a separate JIRA for that?


- Jarek


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


On Nov. 3, 2014, 2:47 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27330/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2014, 2:47 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see JIRA and its parent JIRA for details
> 
> FYI : it does nto include the start/submit and stop/abort renames.
> 
> new tickets have been added to address these
> 
> 
> 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/SubmissionBean.java 4b80338 
>   common/src/main/java/org/apache/sqoop/json/SubmissionsBean.java PRE-CREATION 
>   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/TestSubmissionBean.java e4d50bf 
>   core/src/main/java/org/apache/sqoop/driver/DriverError.java ddee282 
>   core/src/main/java/org/apache/sqoop/driver/JobManager.java ba56c77 
>   core/src/main/java/org/apache/sqoop/driver/JobRequest.java 2666320 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 976223d 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 1e22759 
>   core/src/main/java/org/apache/sqoop/repository/Repository.java 09989e0 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
b324f4f 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaInsertUpdateDeleteSelectQuery.java
c894d06 
>   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/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/ShowJobStatusFunction.java PRE-CREATION

>   shell/src/main/java/org/apache/sqoop/shell/SqoopShell.java 2e87965 
>   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/StopJobFunction.java 790c522 
>   shell/src/main/java/org/apache/sqoop/shell/UpdateJobFunction.java dd075d7 
>   shell/src/main/java/org/apache/sqoop/shell/UpdateLinkFunction.java 176833a 
>   test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java 06462a3 
>   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 
> 
> Diff: https://reviews.apache.org/r/27330/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


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