Skip to content

Conversation

@djkevincr
Copy link
Member

No description provided.

@lewismc
Copy link
Member

lewismc commented Apr 26, 2016

@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

@djkevincr
Copy link
Member Author

@lewismc Thanks for the hints, will make sure those are followed from next time. Please review and let me know if there any concerns.

@lewismc
Copy link
Member

lewismc commented Apr 27, 2016

@renato2099 any comments here?

@renato2099
Copy link
Contributor

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?

@djkevincr
Copy link
Member Author

@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?

@djkevincr
Copy link
Member Author

@lewismc @renato2099 Is there anything which I could help here?

@lewismc
Copy link
Member

lewismc commented May 24, 2016

We need to ensure that the additions to the persistent class/compiler do not render data which has already been written useless!

@djkevincr
Copy link
Member Author

@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 ?

@lewismc
Copy link
Member

lewismc commented May 24, 2016

I am +1 for this patch. Any comments @renato2099. I've tested it locally and it fixes the issue which was previous failing with the Unit test.

lewismc added a commit to lewismc/gora that referenced this pull request May 26, 2016
@asfgit asfgit merged commit 640852e into apache:master May 26, 2016
@lewismc
Copy link
Member

lewismc commented May 26, 2016

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

@renato2099
Copy link
Contributor

Thanks @lewismc @djkevincr ! This looks much more to what we needed :D Awesome work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants