> 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.
Also, this particular change does not require any server side changes.
Change:
printClientResponse(clientResponse);
checkIfSuccessful(clientResponse);
To:
printClientResponse(clientResponse, clazz);
checkIfSuccessful(clientResponse, clazz);
- Pallavi
-----------------------------------------------------------
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
>
>
