ChenSammi commented on pull request #1642: URL: https://github.com/apache/ozone/pull/1642#issuecomment-737927376
> +1 Thanks the patch, @ChenSammi > > My first thought was to suggest using `ExceptionMapper<OMException>` instead of adding this exception handling manually to all places. > > Then I realized the problem: you need to add the context (bucket name, key name) to the results which is possible in this way. This patch does it well --> `+1` > > /brainstorming on > > In the future we may add the context information (bucket, key, volume) to the `OMException` which would make it possible to handle all the exceptions with less code. > > /brainstorming off > > Also, an additional (slightly unrelated) note: if `OzoneClient` itself couldn't be created (due to malformed auth header) still we throw 500 AFAIK. (Not related to this patch, just sharing my sadness ;-) ). Implementation of the design in #1562 may help on that one. > > (Not committing yet, to give a chance to @bharatviswa504 to check this) Thanks @elek for the code review. Yes. currently there is no central place to handle common exceptions, such as permission deny, not leader etc. We can improve this part later by refactoring current code structure. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
