You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gora.apache.org by lewismc <gi...@git.apache.org> on 2015/08/29 23:47:58 UTC
[GitHub] gora pull request: GORA-429 Implement Maven forbidden-apis plugin ...
GitHub user lewismc opened a pull request:
https://github.com/apache/gora/pull/32
GORA-429 Implement Maven forbidden-apis plugin in Gora
Hi Folks, this is a PR for making Gora adhere to the forbidden api's checks. It turned out to be quite a large patch but all of the changes are minor.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lewismc/gora GORA-429
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/gora/pull/32.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 #32
----
commit a303ff796bff390cdf2368dbf2edfaeba33161f5
Author: Lewis John McGibbney <le...@jpl.nasa.gov>
Date: 2015-08-29T21:47:09Z
GORA-429 Implement Maven forbidden-apis plugin in Gora
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] gora pull request: GORA-429 Implement Maven forbidden-apis plugin ...
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on a diff in the pull request:
https://github.com/apache/gora/pull/32#discussion_r38468879
--- Diff: gora-accumulo/src/main/java/org/apache/gora/accumulo/store/AccumuloStore.java ---
@@ -142,7 +142,7 @@ public Object fromBytes(Schema schema, byte data[]) throws GoraException {
data = decoder.readBytes(null).array();
}
} catch (IOException e) {
- e.printStackTrace();
+ LOG.error(e.getMessage());
--- End diff --
LGTM!!! Thanks @lewismc !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] gora pull request: GORA-429 Implement Maven forbidden-apis plugin ...
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on the pull request:
https://github.com/apache/gora/pull/32#issuecomment-136675372
Hi @lewismc
I think the logic is fine, the non-null checks you are describing are for checking nullable schemas, and that is what you've described. Yes, it is recursive if there are nested objects, but the exception is for when "something" goes wrong with serialization (for example, class changes can easily make your serialized data unreadable if you were using reflection) that is why Java forces us to throw an IOException and not NullPointerException. And caching this exception makes us "retry" serialization calling again to <return toBytes(o)> on the object. If we don't print the IO exception, we would probably get an stackOverFlowException because of the recursion and catching this error would be a lot harder.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] gora pull request: GORA-429 Implement Maven forbidden-apis plugin ...
Posted by lewismc <gi...@git.apache.org>.
Github user lewismc commented on a diff in the pull request:
https://github.com/apache/gora/pull/32#discussion_r38287924
--- Diff: gora-accumulo/src/main/java/org/apache/gora/accumulo/store/AccumuloStore.java ---
@@ -248,7 +247,6 @@ public K fromBytes(Class<K> clazz, byte[] val) {
avroEncoder.flush();
return baos.toByteArray();
} catch (IOException e) {
- e.printStackTrace();
--- End diff --
Hi @renato2099 actually, I think the answer is no. We should not be printing an exception here. I _think_ the logic may be flawed.
We make a check in the code for the object being null (in which case we call firstNullSchemaTypeIndex and then process with the avro encoder) or the alternative which is that the object is not null (in which case we call firstNotNullSchemaTypeIndex and then process with the avro encoder).
This method is recursive.
If you look at the very last else statement
```
} else {
return toBytes(o);
}
```
this throws no logging error or stack trace exception right? I think that this logic is OK. It was wrong in the first place.
* to throw an exception would mean that the recursive code could never be called.
* to log an error would be useless and what other state can the object be in other than either 'null' or 'not null', in which case it has already been processed by the preceding statements!
wdyt. Thank you for review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] gora pull request: GORA-429 Implement Maven forbidden-apis plugin ...
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on a diff in the pull request:
https://github.com/apache/gora/pull/32#discussion_r38273525
--- Diff: gora-accumulo/src/main/java/org/apache/gora/accumulo/store/AccumuloStore.java ---
@@ -248,7 +247,6 @@ public K fromBytes(Class<K> clazz, byte[] val) {
avroEncoder.flush();
return baos.toByteArray();
} catch (IOException e) {
- e.printStackTrace();
--- End diff --
So why not printing the exception here? I mean at least we should print out a message saying that something went wrong, or am I missing anything?
Overall looks good to me @lewismc ! Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] gora pull request: GORA-429 Implement Maven forbidden-apis plugin ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/gora/pull/32
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---