> 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.
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. > On Oct. 31, 2014, 1:57 p.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. > > Veena Basavaraj wrote: > 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. I think that we are sending the same JSON structure for given object - e.g. the Job JSON object looks exactly the same in JobBean and JobsBean. The only difference is that JobBean will send the Job JSON structure directly whereas JobsBean will put several Job JSON structures into array. That seems too small difference to me - I feel that it's much easier to simply reuse the JobsBean even when I need to send one Job JSON structure. Your handler s can be simplified as you don't have to know whether server will return only one or multiple, ... > 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. 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. > On Oct. 31, 2014, 1:57 p.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? > > Veena Basavaraj wrote: > 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. On this particular class, we have "action" first and then the object structure from most generic to most concrete. My note was applicable for entire review request, so I've probably picked up wrong line to make the point on, sorry for the confusion. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27330/#review59331 ----------------------------------------------------------- On Oct. 30, 2014, 9: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, 9: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 > >
