You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gora.apache.org by alfonsonishikawa <gi...@git.apache.org> on 2018/01/18 17:46:20 UTC
[GitHub] gora pull request #127: GORA-530 : Reinstated exception throwing in DataStor...
GitHub user alfonsonishikawa opened a pull request:
https://github.com/apache/gora/pull/127
GORA-530 : Reinstated exception throwing in DataStore and Query
https://issues.apache.org/jira/browse/GORA-530
This pull request reinstates exceptions throwing in DataStore and Query.
I updated it to throw `GoraException` at 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 :package:
Thanks!
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alfonsonishikawa/gora GORA-530
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/gora/pull/127.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #127
----
commit b06da5f32ec572c88f7ec5245a4b573c73ae8c22
Author: Alfonso Nishikawa Muñumer <al...@...>
Date: 2018-01-18T17:37:09Z
GORA-530 : Reinstated exception throwing in DataStore and Query
----
---
[GitHub] gora pull request #127: GORA-530 : Reinstated exception throwing in DataStor...
Posted by alfonsonishikawa <gi...@git.apache.org>.
Github user alfonsonishikawa commented on a diff in the pull request:
https://github.com/apache/gora/pull/127#discussion_r168014313
--- Diff: gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeStore.java ---
@@ -159,17 +167,25 @@ public boolean schemaExists() {
* @return the Object corresponding to the key or null if it cannot be found
*/
@Override
- public T get(K key, String[] fields) {
-
- Key recordKey = getAerospikeKey(key);
- fields = getFieldsToQuery(fields);
+ public T get(K key, String[] fields) throws GoraException {
- Record record = aerospikeClient
- .get(aerospikeParameters.getAerospikeMapping().getReadPolicy(), recordKey, fields);
- if (record == null) {
- return null;
+ try {
+ Key recordKey = getAerospikeKey(key);
+ fields = getFieldsToQuery(fields);
+
+ Record record = aerospikeClient
+ .get(aerospikeParameters.getAerospikeMapping().getReadPolicy(), recordKey, fields);
+
+ if (record == null) {
+ return null;
+ }
+ return createPersistentInstance(record, fields);
+ } catch (GoraException e) {
+ throw e;
--- End diff --
Hi! Yes. Notice that createPersistentInstance has been updated to throw GoraException. Throughout the changes, 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 wraping 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!
---
[GitHub] gora pull request #127: GORA-530 : Reinstated exception throwing in DataStor...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/gora/pull/127
---
[GitHub] gora issue #127: GORA-530 : Reinstated exception throwing in DataStore and Q...
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on the issue:
https://github.com/apache/gora/pull/127
@alfonsonishikawa Thanks for doing this!
---
[GitHub] gora issue #127: GORA-530 : Reinstated exception throwing in DataStore and Q...
Posted by alfonsonishikawa <gi...@git.apache.org>.
Github user alfonsonishikawa commented on the issue:
https://github.com/apache/gora/pull/127
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 :)
---
[GitHub] gora pull request #127: GORA-530 : Reinstated exception throwing in DataStor...
Posted by alfonsonishikawa <gi...@git.apache.org>.
Github user alfonsonishikawa commented on a diff in the pull request:
https://github.com/apache/gora/pull/127#discussion_r171598778
--- Diff: gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeStore.java ---
@@ -159,17 +167,25 @@ public boolean schemaExists() {
* @return the Object corresponding to the key or null if it cannot be found
*/
@Override
- public T get(K key, String[] fields) {
-
- Key recordKey = getAerospikeKey(key);
- fields = getFieldsToQuery(fields);
+ public T get(K key, String[] fields) throws GoraException {
- Record record = aerospikeClient
- .get(aerospikeParameters.getAerospikeMapping().getReadPolicy(), recordKey, fields);
- if (record == null) {
- return null;
+ try {
+ Key recordKey = getAerospikeKey(key);
+ fields = getFieldsToQuery(fields);
+
+ Record record = aerospikeClient
+ .get(aerospikeParameters.getAerospikeMapping().getReadPolicy(), recordKey, fields);
+
+ if (record == null) {
+ return null;
+ }
+ return createPersistentInstance(record, fields);
+ } catch (GoraException e) {
+ throw e;
--- End diff --
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!
---
[GitHub] gora issue #127: GORA-530 : Reinstated exception throwing in DataStore and Q...
Posted by lewismc <gi...@git.apache.org>.
Github user lewismc commented on the issue:
https://github.com/apache/gora/pull/127
I @alfonsonishikawa I think we should go ahead and merge this into master branch. It been sitting for long enough. +1
---
[GitHub] gora pull request #127: GORA-530 : Reinstated exception throwing in DataStor...
Posted by nishadi <gi...@git.apache.org>.
Github user nishadi commented on a diff in the pull request:
https://github.com/apache/gora/pull/127#discussion_r167853773
--- Diff: gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeStore.java ---
@@ -159,17 +167,25 @@ public boolean schemaExists() {
* @return the Object corresponding to the key or null if it cannot be found
*/
@Override
- public T get(K key, String[] fields) {
-
- Key recordKey = getAerospikeKey(key);
- fields = getFieldsToQuery(fields);
+ public T get(K key, String[] fields) throws GoraException {
- Record record = aerospikeClient
- .get(aerospikeParameters.getAerospikeMapping().getReadPolicy(), recordKey, fields);
- if (record == null) {
- return null;
+ try {
+ Key recordKey = getAerospikeKey(key);
+ fields = getFieldsToQuery(fields);
+
+ Record record = aerospikeClient
+ .get(aerospikeParameters.getAerospikeMapping().getReadPolicy(), recordKey, fields);
+
+ if (record == null) {
+ return null;
+ }
+ return createPersistentInstance(record, fields);
+ } catch (GoraException e) {
+ throw e;
--- End diff --
Hi,
Is there any specific reason why we are capturing the GoraException separately?
---