> 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);
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. - Ajay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41587/#review111437 ----------------------------------------------------------- On Dec. 28, 2015, 2:07 p.m., Praveen Adlakha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41587/ > ----------------------------------------------------------- > > (Updated Dec. 28, 2015, 2:07 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 > ----- > > 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 aea39a6 > 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 > >
