> On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > server/src/main/java/org/apache/sqoop/handler/ConfigInputRequestHandler.java, > > lines 127-129 > > <https://reviews.apache.org/r/31173/diff/1/?file=868592#file868592line127> > > > > default case?
there is no need since if one of the expected is not given an exception is already thrown if you see the code above it should become clear > On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > server/src/main/java/org/apache/sqoop/handler/ConfigInputRequestHandler.java, > > line 114 > > <https://reviews.apache.org/r/31173/diff/1/?file=868592#file868592line114> > > > > default case? there is no need since if one of the expected is not given an exception is already thrown if you see the code above it should become clear > On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > server/src/main/java/org/apache/sqoop/handler/ConfigInputRequestHandler.java, > > line 139 > > <https://reviews.apache.org/r/31173/diff/1/?file=868592#file868592line139> > > > > provide default cases? there is no need since if one of the expected is not given an exception is already thrown if you see the code above it should become clear > On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > server/src/main/java/org/apache/sqoop/handler/ConfigInputRequestHandler.java, > > line 164 > > <https://reviews.apache.org/r/31173/diff/1/?file=868592#file868592line164> > > > > if config is null, JobConfigInputBean functions will throw NPE. such checks are not done in other beans as well, and NPE happen in every single place, even when one of the attribute inside config is missing. So probably it was not required > On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > server/src/main/java/org/apache/sqoop/handler/ConfigInputRequestHandler.java, > > lines 73-75 > > <https://reviews.apache.org/r/31173/diff/1/?file=868592#file868592line73> > > > > Throw an exception if we don't support the method? this chek is already made in the beginnning to support GET and PUT Only, so this cannot happen, the compiler needs to have this to avoid a warning > On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > common/src/main/java/org/apache/sqoop/json/JobConfigInputBean.java, lines > > 69-75 > > <https://reviews.apache.org/r/31173/diff/1/?file=868583#file868583line69> > > > > Default case that throws a runtime exception? Seems like this case > > should result in the user seeing a 400 error. > > > > Also, NPE if this is null... which seems possible right now. there is no need since if one of the expected is not given an exception is already thrown if you see the code above it should become clear > On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > common/src/main/java/org/apache/sqoop/json/JobConfigInputBean.java, lines > > 55-61 > > <https://reviews.apache.org/r/31173/diff/1/?file=868583#file868583line55> > > > > Default case? Or just return an empty object is sufficient? Maybe throw > > a runtime exception? there is no need since if one of the expected is not given an exception is already thrown if you see the code above it should become clear > On Feb. 18, 2015, 11:42 p.m., Abraham Elmahrek wrote: > > common/src/main/java/org/apache/sqoop/json/JobConfigInputBean.java, line 55 > > <https://reviews.apache.org/r/31173/diff/1/?file=868583#file868583line55> > > > > Not sure if calls like this work with SQOOP-1516. FROM_SUB_TYPE doesn't > > exist? it is in the MConfigTyoe, since I cannot upload this RB until you commit the 1516, I had to remove that file in this RB, makes sense? if not I can explain it more details I tried to get an RB out at the earliest for feedback - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31173/#review73095 ----------------------------------------------------------- On Feb. 18, 2015, 5:15 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31173/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2015, 5:15 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2113 > https://issues.apache.org/jira/browse/SQOOP-2113 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see jira > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/error/code/ServerError.java > 86cca7dcc9c7ea0f69b279dd85d3e68a1eda6596 > common/src/main/java/org/apache/sqoop/json/JobBean.java > 0561adebd3b604b27512e22227db076712f27fd0 > common/src/main/java/org/apache/sqoop/json/JobConfigInputBean.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/json/JsonBean.java > 1dd275e80df25b3521bb20dd31e4e2221db50ff8 > common/src/main/java/org/apache/sqoop/json/LinkBean.java > 2e2406f3de35294896e280b57d505197a6d6af86 > common/src/main/java/org/apache/sqoop/json/LinkConfigInputBean.java > PRE-CREATION > > common/src/main/java/org/apache/sqoop/json/util/ConfigInputSerialization.java > ffaf041baed67974b76f3b789b14414ec487a0a4 > common/src/main/java/org/apache/sqoop/model/ConfigUtils.java > dd6e9ce481c12b7d096f4e8473acbdb2f04eee90 > common/src/test/java/org/apache/sqoop/json/TestJobConfigInputBean.java > PRE-CREATION > common/src/test/java/org/apache/sqoop/json/TestLinkConfigInputBean.java > PRE-CREATION > common/src/test/java/org/apache/sqoop/json/util/ConfigTestUtil.java > fbc7faa3a24810952fd22d30b69ee1e983e04547 > > server/src/main/java/org/apache/sqoop/handler/ConfigInputRequestHandler.java > PRE-CREATION > server/src/main/java/org/apache/sqoop/server/v1/ConfigInputServlet.java > PRE-CREATION > server/src/main/webapp/WEB-INF/web.xml > 60ee8c4acace2aaab8909f5c6daa6600875f7794 > > Diff: https://reviews.apache.org/r/31173/diff/ > > > Testing > ------- > > done > > > Thanks, > > Veena Basavaraj > >
