falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajay Yadava" <ajayn...@gmail.com>
Subject Re: Review Request 41587: Listing API non-intuitive response if time > endTime
Date Tue, 29 Dec 2015 09:18:00 GMT


> 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.

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.


- 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message