You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Praveen Adlakha <ad...@gmail.com> on 2015/12/19 10:19:53 UTC
Review Request 41587: Listing API non-intuitive response if time >
endTime
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/
-----------------------------------------------------------
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 b4b1762
prism/src/main/java/org/apache/falcon/FalconWebException.java 7324997
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
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
> On Dec. 21, 2015, 5:06 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/channel/HTTPChannel.java, line 127
> > <https://reviews.apache.org/r/41587/diff/1/?file=1172723#file1172723line127>
> >
> > Generally, the web exception is thrown by the JAX-RS layer, rather than the resource layer.
It is available for extension and can be used for error scenarios. Falcon follows this convention to throw error messages.
> On Dec. 21, 2015, 5:06 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/proxy/InstanceManagerProxy.java, line 622
> > <https://reviews.apache.org/r/41587/diff/1/?file=1172724#file1172724line622>
> >
> > the catch clause for Throwable is already handling all exceptions. Better to change there?
Both of them have different handling.
> On Dec. 21, 2015, 5:06 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java, line 880
> > <https://reviews.apache.org/r/41587/diff/1/?file=1172722#file1172722line880>
> >
> > Generally, the web exception is thrown by the JAX-RS layer, rather than the resource layer.
It is available for extension and can be used for error scenarios. Falcon follows this convention to throw error messages. It is not something newly added in this JIRA.
- Ajay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/#review111437
-----------------------------------------------------------
On Dec. 19, 2015, 9:19 a.m., Praveen Adlakha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41587/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2015, 9:19 a.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 b4b1762
> prism/src/main/java/org/apache/falcon/FalconWebException.java 7324997
> 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
> 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.
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.
- 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Pallavi Rao <pa...@inmobi.com>.
> 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.
Sorry.. I meant if someone adds a new common parameter to sendInstanceRequest, then they'll also have to modify feedListing method.
- 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Pallavi Rao <pa...@inmobi.com>.
> 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.
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.
- 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
> 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Pallavi Rao <pa...@inmobi.com>.
> 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.
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
-----------------------------------------------------------
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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
> 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Pallavi Rao <pa...@inmobi.com>.
> 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.
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.
- 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Pallavi Rao <pa...@inmobi.com>.
> 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
> 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.
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.
> On Dec. 21, 2015, 5:06 a.m., Pallavi Rao wrote:
> > client/src/main/java/org/apache/falcon/client/FalconCLIException.java, line 80
> > <https://reviews.apache.org/r/41587/diff/1/?file=1172719#file1172719line80>
> >
> > Can't we replace the existing methods with these, rather than adding new ones?
Yes, we should have only one method. 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.
> On Dec. 21, 2015, 5:06 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java, line 531
> > <https://reviews.apache.org/r/41587/diff/1/?file=1172722#file1172722line531>
> >
> > Why not modify the newInstanceException method rather than use a generic newAPIException method?
I had an offline discussion with @Praveen Adlakha on the same. I think the approach warrants a few clarifications. He is doing it this way to make the change unobtrusive and at the same time make it for 0.9 release without messing around with too many API calls. Changing existing methods was causing cascading changes in several other API calls. He wants to handle them in a separate JIRA.
- 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
- 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
> 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Pallavi Rao <pa...@inmobi.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/#review111437
-----------------------------------------------------------
client/src/main/java/org/apache/falcon/cli/FalconInstanceCLI.java
<https://reviews.apache.org/r/41587/#comment171610>
Why is doAsUser removed?
client/src/main/java/org/apache/falcon/client/FalconCLIException.java (line 80)
<https://reviews.apache.org/r/41587/#comment171616>
Can't we replace the existing methods with these, rather than adding new ones?
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 949)
<https://reviews.apache.org/r/41587/#comment171611>
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.
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 955)
<https://reviews.apache.org/r/41587/#comment171612>
nit : Better not leave commented code behind.
prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java (line 530)
<https://reviews.apache.org/r/41587/#comment171618>
Why remove logging?
prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java
<https://reviews.apache.org/r/41587/#comment171619>
Why not modify the newInstanceException method rather than use a generic newAPIException method?
prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java (line 879)
<https://reviews.apache.org/r/41587/#comment171621>
Generally, the web exception is thrown by the JAX-RS layer, rather than the resource layer.
prism/src/main/java/org/apache/falcon/resource/channel/HTTPChannel.java (line 36)
<https://reviews.apache.org/r/41587/#comment171613>
This will result in checkstyle errors. Expand the imports, please. Guess you'll need to change your IDE preferencens to not automatically collapse.
prism/src/main/java/org/apache/falcon/resource/channel/HTTPChannel.java (line 48)
<https://reviews.apache.org/r/41587/#comment171615>
whitespace / format differences with the code base and your IDE prefs.
prism/src/main/java/org/apache/falcon/resource/channel/HTTPChannel.java (line 120)
<https://reviews.apache.org/r/41587/#comment171622>
Generally, the web exception is thrown by the JAX-RS layer, rather than the resource layer.
prism/src/main/java/org/apache/falcon/resource/proxy/InstanceManagerProxy.java (line 622)
<https://reviews.apache.org/r/41587/#comment171623>
the catch clause for Throwable is already handling all exceptions. Better to change there?
- Pallavi Rao
On Dec. 19, 2015, 9:19 a.m., Praveen Adlakha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41587/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2015, 9:19 a.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 b4b1762
> prism/src/main/java/org/apache/falcon/FalconWebException.java 7324997
> 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Praveen Adlakha <ad...@gmail.com>.
> On Dec. 21, 2015, 6:25 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/client/FalconCLIException.java, line 93
> > <https://reviews.apache.org/r/41587/diff/1/?file=1172719#file1172719line93>
> >
> > Just curious, is it possible to decode the class to be used from status code?
No we cannot get this info at this level.
- Praveen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/#review111441
-----------------------------------------------------------
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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/#review111441
-----------------------------------------------------------
client/src/main/java/org/apache/falcon/client/FalconCLIException.java (line 80)
<https://reviews.apache.org/r/41587/#comment171635>
Please add commen on other versions of this method to indicate others to use this method.
client/src/main/java/org/apache/falcon/client/FalconCLIException.java (line 81)
<https://reviews.apache.org/r/41587/#comment171626>
Ideally, there should be only one way to do it but I assume you are not changing old methods and just adding new one for this API, to limit the scope of the change.
client/src/main/java/org/apache/falcon/client/FalconCLIException.java (line 84)
<https://reviews.apache.org/r/41587/#comment171625>
If you are planning to migrate in phases, then it will be nice to add a comment on this method to not be used and developers should use the method above it?
client/src/main/java/org/apache/falcon/client/FalconCLIException.java (line 88)
<https://reviews.apache.org/r/41587/#comment171634>
I suspect status is already checked before calling this method.
client/src/main/java/org/apache/falcon/client/FalconCLIException.java (line 93)
<https://reviews.apache.org/r/41587/#comment171628>
Just curious, is it possible to decode the class to be used from status code?
client/src/main/java/org/apache/falcon/client/FalconCLIException.java (line 109)
<https://reviews.apache.org/r/41587/#comment171627>
Incorrect indentation?
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 956)
<https://reviews.apache.org/r/41587/#comment171629>
You need to add null checks for query params.
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 958)
<https://reviews.apache.org/r/41587/#comment171630>
You need to add the "end" parameter as well after checking for null.
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 1258)
<https://reviews.apache.org/r/41587/#comment171631>
Please add a comment on the other version of this method to point users to use this method with appropriate reasons.
prism/src/main/java/org/apache/falcon/FalconWebException.java (line 108)
<https://reviews.apache.org/r/41587/#comment171632>
I think we should delete other class specific versions and consolidate exception creation.
prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java (line 520)
<https://reviews.apache.org/r/41587/#comment171633>
Please log the entityType which was passed along with the message.
- Ajay Yadava
On Dec. 19, 2015, 9:19 a.m., Praveen Adlakha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41587/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2015, 9:19 a.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 b4b1762
> prism/src/main/java/org/apache/falcon/FalconWebException.java 7324997
> 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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/#review112120
-----------------------------------------------------------
Praveen Adlakha,
Can you please close all the earlier review comments?
- Ajay Yadava
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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/#review112208
-----------------------------------------------------------
CHANGES.txt (line 93)
<https://reviews.apache.org/r/41587/#comment172550>
Any changes in CHANGES.txt will increase your chances of conflict and patch won't apply cleanly. Due to this reason it's generally best idea to leave this to the person committing the patch.
- Ajay Yadava
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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/#review112211
-----------------------------------------------------------
Ship it!
Ship It!
- Ajay Yadava
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
>
>
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Praveen Adlakha <ad...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Praveen Adlakha <ad...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 41587: Listing API non-intuitive response if time >
endTime
Posted by Praveen Adlakha <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41587/
-----------------------------------------------------------
(Updated Dec. 28, 2015, 1:17 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 (updated)
-----
client/src/main/java/org/apache/falcon/client/FalconCLIException.java 51ef952
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