> On Dec. 10, 2014, 8:44 a.m., Gwen Shapira wrote: > > Thank you so much for fixing it. > > > > Few comments: > > * The change modifies the JSON returned when getting submission from REST > > API. I'd expect a documentation change too. > > * SubmissionBean changes, so the client API changed too - again, we need to > > document the new behavior > > * I'm concerned about the repository changes - what will happen with > > submissions already stored in the DB? Is the API change backwards > > compatible? Will be be able to read old submissions? If it is, perhaps add > > a unit test to show compatibility? > > * The tests are only for exceptions and traces, but I thought part of the > > change was to support errors without exceptions? Don't we need to test > > those? > > Veena Basavaraj wrote: > - back compat with JSON is not been maintained since many releases and I > dont know if this matters much since the only client is HUE and it is not > even ready > > - happy to add a doc ticket for the naming change once the code is in I > can fix the doc > > - the schema will still store the same 2 fields in the existing table, > might be a good idea to rename the fields in > - | SQS_EXCEPTION: VARCHAR(150) | > * | SQS_EXCEPTION_TRACE: VARCHAR(750) | > * +---------------------------------- > > > The change unifies the failure and exception into one thing called > error. So not sure what testing is required there > > > The integration tests pass so this means the fields are stored and > retrieved in the submission, not sure unti tests for compatibility check > makes sense, have not done that, would you mind iterating your thoguhts on > that? > > Gwen Shapira wrote: > * I'm ok with not maintaining back compat for JSON for now. > > * The backward compat I'm concerned about is: What will happen if we have > existing submissions in the DB and use the new API to request them? will we > be able to read them? I'd like to see a test around that. > > * Any reason not to add doc changes in this patch? It gives us > "atomicity" for the change. (this is not specific for this patch, I insist on > docs inclusion in all patches I review. I think its the right practice) > > * It will be clearer to rename, but then we'll have to worry about > upgrades. I'll let you make the call here. > > * failure and exception are integrated into one error. I'd like to see > tests (if possible) showing that we successfully grab non-exception failures > into the field. I'm not sure we have those right now. > > Hope this is clearer?
I 'd rather have separate tickets and patches for doc and schema renames As I said before integration tests passes, so this means it can read and write a submission. Let me know if you need more details on how the integration is testing the submission code path. rename means upgrade as well, its implicit since I have tons of this in the past.:) can you elaborate which part ? is that a unit or integration test, since the failure is a MR job related data. I must be missing soemthing there? - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28676/#review64563 ----------------------------------------------------------- On Dec. 8, 2014, 2:40 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28676/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2014, 2:40 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1839 > https://issues.apache.org/jira/browse/SQOOP-1839 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see jira > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/json/SubmissionBean.java e926f02 > common/src/main/java/org/apache/sqoop/model/MSubmission.java 2648712 > common/src/main/java/org/apache/sqoop/model/SubmissionError.java > PRE-CREATION > common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d07eda9 > core/src/main/java/org/apache/sqoop/driver/JobManager.java f4f5561 > core/src/main/java/org/apache/sqoop/driver/JobRequest.java eed79a5 > core/src/main/java/org/apache/sqoop/driver/SubmissionEngine.java 3a32e9f > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java > c278406 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java > e2e8073 > shell/src/main/java/org/apache/sqoop/shell/ShowSubmissionFunction.java > 4a0f476 > shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java > 0e2a38d > > submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java > 631ceca > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > 4f5f52a > > Diff: https://reviews.apache.org/r/28676/diff/ > > > Testing > ------- > > > Thanks, > > Veena Basavaraj > >
