You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gora.apache.org by djkevincr <gi...@git.apache.org> on 2016/04/25 15:26:50 UTC
[GitHub] gora pull request: GORA_447 NPE Fix
GitHub user djkevincr opened a pull request:
https://github.com/apache/gora/pull/61
GORA_447 NPE Fix
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/djkevincr/gora GORA_225
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/gora/pull/61.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 #61
----
commit b7fb9eea768a6daac93d1835726c737153f45ef4
Author: Kevin Ratnasekera <dj...@yahoo.com>
Date: 2016-04-25T13:21:58Z
GORA_447 NPE Fix
----
---
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_447 NPE Fix
Posted by lewismc <gi...@git.apache.org>.
Github user lewismc commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-221259954
We need to ensure that the additions to the persistent class/compiler do not render data which has already been written useless!
---
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_447 NPE Fix
Posted by djkevincr <gi...@git.apache.org>.
Github user djkevincr commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-214959860
@lewismc Thanks for the hints, will make sure those are followed from next time. Please review and let me know if there any concerns.
---
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_447 NPE Fix
Posted by djkevincr <gi...@git.apache.org>.
Github user djkevincr commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-221264044
@lewismc +1 I have removed all the changes I did to Persistent class/compiler as per the @renato2099 review. I added some changes to deleteByQuery(Query<K, T> query) and getPersistent(T obj, String[] fields) of MemStore. Further I added some unit tests for getPersistent(T obj, String[] fields) to MemStoreTest. Can you please point me to the code if anything I am missing here ?
---
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_447 NPE Fix
Posted by lewismc <gi...@git.apache.org>.
Github user lewismc commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-215107783
@renato2099 any comments here?
---
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_447 NPE Fix
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-221799177
Thanks @lewismc @djkevincr ! This looks much more to what we needed :D Awesome work!
---
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_447 NPE Fix
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/gora/pull/61
---
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_447 NPE Fix
Posted by lewismc <gi...@git.apache.org>.
Github user lewismc commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-221270919
I am +1 for this patch. Any comments @renato2099
---
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_447 NPE Fix
Posted by djkevincr <gi...@git.apache.org>.
Github user djkevincr commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-215446689
@renato2099 +1, I also agree with your concerns. However I had to make some changes to MemStore getPersistent(T obj, String[] fields) to work as expected. I have further added some test cases to verify this method. Can you please have look?
---
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_447 NPE Fix
Posted by lewismc <gi...@git.apache.org>.
Github user lewismc commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-221791522
Thank you @djkevincr if you could
1) name your commit message and hence Github issue after the Jira ticket, it means the Github hooks work perfectly.
2) Squash your commits into one if possible.
Thank you very much @djkevincr
---
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_447 NPE Fix
Posted by renato2099 <gi...@git.apache.org>.
Github user renato2099 commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-215258302
it's pretty cool stuff @djkevincr ! But I think we need to go over it. My main concern is the addition of the clearField method to the Persistent class. Two main thoughts on it:
- We should try to keep the main Persistent API as stable as possible because we have some dependencies on it e.g. goraCompiler, and therefore all the avro generated beans we create from it.
- This method is proposed for clearing the values of MemStore. So it would seem that only MemStore needs it. So I am not sure we need to add it to main API. Why don't we use a method like getPersistent? What do you guys think @lewismc @djkevincr ?
The other thing that I think we need to change is how the deleteByQuery is working now. I mean it's great that is not breaking anymore :D but I think we need and IfElse stmt after the if(allFields) because if we have deleted the specific key, then why do we need to iterate through the fields?
---
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_447 NPE Fix
Posted by djkevincr <gi...@git.apache.org>.
Github user djkevincr commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-221124233
@lewismc @renato2099 Is there anything which I could help here?
---
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_447 NPE Fix
Posted by lewismc <gi...@git.apache.org>.
Github user lewismc commented on the pull request:
https://github.com/apache/gora/pull/61#issuecomment-214878365
@djkevincr thanks for this PR.
Some hints; when you open an issue here and make your commit message, if you are able to make the JIRA issue and Commit message the same, then all correspondence will be shadowed over to the JIRA ticket automatically.
I'll check this PR out ASAP. 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.
---