GORA-530 : Reinstated exception throwing in DataStore and Query #127
Conversation
|
@alfonsonishikawa Thanks for doing this! |
…schema does not exist
|
Updated the pull request with a fix for a little bug I introduced. AccumuloStore#deleteSchema() was failing when the table did not exist. Not does not fail. Thanks in advance for reviewing the datastore/s you know :) |
| } | ||
| return createPersistentInstance(record, fields); | ||
| } catch (GoraException e) { | ||
| throw e; |
There was a problem hiding this comment.
Hi,
Is there any specific reason why we are capturing the GoraException separately?
There was a problem hiding this comment.
Hi! Yes. Notice that createPersistentInstance() has been updated to throw GoraException. Throughout this diff, GoraException is being used as a simple wrapper to have a common interface for all datastores, so as you can see two lines bellow in this diff, any exception (not GoraException) is logged and wrapped. In the case of a catch of a GoraException we can safely assume that it is already logged and wrapping other exception (or maybe an actual GoraException but this does not make difference) and we wouldn't want to wrap it again because that would be useless and would hide the exception's parent exception in an indefinite sequence.
Maybe it is not the best solution, but I didn't get to any better. If you have any suggestion it will be welcome!
There was a problem hiding this comment.
Hi, Nishadi.
I have been thinking about this. You asked because in the case the is only a log and a throw? I actually just enclosed with try-catch whatever existed before without thinking much about it. But if you are saying that we should only throw the exceptions without logging -since we are not logging anything interesting-, then I guess you are right. I could have done that, but I just did not want to change much the existing datastores :P My fault. It would be good just throw the exceptions and nothing more.
I will see about changing it whenever I have the time and energy :) Or if anyone wants to change it...
Thanks!
|
I @alfonsonishikawa I think we should go ahead and merge this into master branch. It been sitting for long enough. +1 |
https://issues.apache.org/jira/browse/GORA-530
This pull request reinstates exceptions throwing in DataStore and Query.
I updated it to throw
GoraExceptionat several methods.Updated tests because I found little inconsistencies after starting throwing exceptions (tests failing).
Please, review the datastores and comment anything you see along, because I made the changes and tests pass but I only know HBaseStore. It has been a pain to change all this in a bulk 📦
Thanks!