> On Dec. 21, 2015, 5:06 a.m., Pallavi Rao wrote: > > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 949 > > <https://reviews.apache.org/r/41587/diff/1/?file=1172720#file1172720line949> > > > > Let us not introduce a new method. I believe you introduced this in > > order to supply FeedInstanceResult to appropriate methods. > > > > It would be better to refactor the code to supply the appropriate class > > in order methods too. > > Ajay Yadava wrote: > I had an offline discussion with Praveen Adlakha. The reason he took this > approach is because that one method is used in several apis. He wants to > tackle all other APIs in a separate JIRA as they will potentially entail > changes on both client side and server side. > > Pallavi Rao wrote: > Introducing another utility method and updating the code to use this new > method (as in case of FalconCLIException) may be acceptable to do in 2 > separate JIRAs. But, introduction of a new and duplicate API is confusing and > error prone. IMO, any changes to keep a single method must be handled in the > same JIRA. For example, if someone has to add a new parameter to > getFeedListing, they'll have to do it in 2 places. > > Ajay Yadava wrote: > I didn't understand why changes will be required at two places, if > someone adds a new parameter to feed listing API they will need to make > change only in one place - getFeedInstanceListing. > > Pallavi Rao wrote: > Sorry.. I meant if someone adds a new common parameter to > sendInstanceRequest, then they'll also have to modify feedListing method. > > Ajay Yadava wrote: > I think the current way of passing all calls through sendInstanceRequest > is a very wrong way of doing things. Currently, if I add a new parameter to > only one of the apis then I will have to change the signature of that method > and that will require to fix all invocations of that method in all APIs by > passing a null. This also results in a very long list of arguments among lot > of other issues. > > Coming to the particular scenario where someone will need to make a > change in both sendInstanceRequest and feedListing method then I think that > will be required only when someone needs to change all API methods, something > like doAsUser. It is not very frequent but in that case the developers will > need to make changes at several places as they want to make changes to all > APIs. Even if we artificially restrict it to one method, same change will be > required in multiple places on server side also. > > Pallavi Rao wrote: > Whether routing all calls through sendInstanceRequest is a different > question which should be reserved for a separate JIRA. > > But, currently, it is duplication of code. All that needs to be done is, > add another parameter to sendInstanceRequest that takes in the class. If > class is not specified, do what is done now, else use the class . Not a big > change to park for another JIRA. > > Pallavi Rao wrote: > Also, this particular change does not require any server side changes. > Change: > printClientResponse(clientResponse); > checkIfSuccessful(clientResponse); > > To: > printClientResponse(clientResponse, clazz); > checkIfSuccessful(clientResponse, clazz); > > Ajay Yadava wrote: > Pallavi I suspect you have got wrong impression of the change. There is > no duplication in the code. The new method is only for one API call and that > API call only uses this method. This approach already exists for several api > calls in the client even today. > > Even if the code were to be changed to go to sendInstanceRequest if no > class is specified(never happens) then it will not avoid changes in case of > adding a new param but will actually increase the work. I will sync with you > offline to explain the change. > > Pallavi Rao wrote: > My understanding was that the sendInstanceRequest was the common method > to send all instance related requests. If that is not the case, and if each > method is handling its request separately, then, it is a different matter. > The class unfortunately is a hotch-potch of handling it in a single common > method and each-API-to-itself. > > Given that, going either way is fine.
I have requested Praveen to try to fix the other JIRAs for improving error handling also in time for 0.9 release. Will commit it in current form. - Ajay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41587/#review111437 ----------------------------------------------------------- On Dec. 29, 2015, 3:36 p.m., Praveen Adlakha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41587/ > ----------------------------------------------------------- > > (Updated Dec. 29, 2015, 3:36 p.m.) > > > Review request for Falcon. > > > Bugs: falcon-1565 > https://issues.apache.org/jira/browse/falcon-1565 > > > Repository: falcon-git > > > Description > ------- > > Listing API non-intuitive response if time > endTime > > > Diffs > ----- > > CHANGES.txt e268ce4 > client/src/main/java/org/apache/falcon/cli/FalconInstanceCLI.java 6ea34da > client/src/main/java/org/apache/falcon/client/FalconCLIException.java > 51ef952 > client/src/main/java/org/apache/falcon/client/FalconClient.java 00aa167 > prism/src/main/java/org/apache/falcon/FalconWebException.java 666e3d7 > prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java > da2fea7 > prism/src/main/java/org/apache/falcon/resource/channel/HTTPChannel.java > 6801398 > > prism/src/main/java/org/apache/falcon/resource/proxy/InstanceManagerProxy.java > 96c99f0 > > Diff: https://reviews.apache.org/r/41587/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Praveen Adlakha > >
