GEODE-6973: Use cachelistener to synchronize typeToId with IdToType by gesterzhou · Pull Request #4014 · apache/geode

gesterzhou · 2019-09-05T00:15:31Z

    Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>
    Co-authored-by: Donal Evans <doevans@pivotal.io>

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically develop)?
Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io> Co-authored-by: Donal Evans <doevans@pivotal.io>

Co-authored-by: Donal Evans <doevans@pivotal.io> Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>

…' into feature/GEODE-6973-2

jhuynh1 · 2019-09-09T16:27:00Z

      return newType.getTypeId();
    } finally {
+      // flush the tmpTypeToId map for who introduced this new pdxType
+      if (!tmpTypeToId.isEmpty()) {


Why is this finally needed? How would the tmpTypeToId not already be empty here?

We have discussed that, it's safe to do it when the creator holds the d-lock.
The listener will only add into tmpTypeToId map now. We use it to make sure that the distribution should all finished before flushing anything into typeToId map (they stayed in tmpTypeToId map now)

But wouldn't the line above (line 386) when we actually use the map, have cleared it? Why do we need to reclear in the finally? Why is the duplicated code needed in the finally block? Same goes for the other method...

bschuchardt

Is it possible to use variable names that don't begin with "tmp" and are more descriptive of what you're accomplishing? It seems like you're staging additions to other collections so maybe "pending" instead of "tmp" would be better.

gesterzhou · 2019-09-09T18:36:45Z

I changed the map's name based on bruce's suggestion.

The map name is changed to "pending"

bschuchardt

I'm sorry but I find this whole change set difficult to follow and think it would be pretty much impossible for a new developer to figure out. Is there any way to make this be less piecemeal, with one method throwing stuff into a collection that it's "understood" that another method will consume under some lock/synchronization? PDX already has confusing caches and this just seems to exacerbate the situation.

Co-authored-by: Donal Evans <doevans@pivotal.io> Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>

Authored-by: Donal Evans <doevans@pivotal.io>

upthewaterspout · 2019-09-09T23:19:48Z

@bschuchardt - To make your suggestion more concrete - would it help to move pendingTypeToId and typeToId to a separate data structure that encapsulates that functionality?

IMO this whole pendingTypeToId business is essentially working around a limitation in our transactions. I would expect that if a CacheListener fires in Member A as a result of a transaction, then a get in Member B after that CacheListener event should see the change. That is unfortunately not the case.

Authored-by: Donal Evans <doevans@pivotal.io>

upthewaterspout

Where did all of this sample json come from? It has email address, urls in it?

How likely is it for some of these new tests to be flaky? I see things like toleratedCollisionFraction=0.01f? Is this a test with a lot of random numbers that may occasionally fail? Can we at least lock it down with a seed so that it either passes or fails consistently?

bschuchardt · 2019-09-09T23:26:52Z

@upthewaterspout yes, that would help. It's like we're writing to a structure but don't want to commit the changes in the current thread. The changes need to be committed in another thread. Encapsulating the collections in another object that requires that the correct locks are obtained before transferring from pending to committed would go a long way toward making these changes more understandable without hearing Gester's presentation on the solution.

Authored-by: Donal Evans <doevans@pivotal.io>

DonalEvans · 2019-09-09T23:31:40Z

@upthewaterspout

I removed those tests. You're right, they were potentially flaky and not testing any behaviour that wasn't accounted for elsewhere. As for where the sample JSON came from, it was just an edited version of an existing JSON resource used in other tests.

gesterzhou · 2019-09-09T23:55:27Z

The testJSON.txt is from ./geode-core/src/distributedTest/resources/org/apache/geode/pdx/jsonStrings/json4.txt.

We can change it to dummy url and email.

gesterzhou · 2019-09-10T00:08:11Z

I will create an inner class LocalReverseMap to wrap the typeToId, enumToId, pendingTypeToId, pendingEnumToId.

gesterzhou · 2019-09-10T05:21:40Z

I created an inner class to wrap all these local maps. Pls review again.

bschuchardt

that's much better - thank you

Authored-by: Donal Evans <doevans@pivotal.io>

…ending map

…ToType (#4014)" This reverts commit cdca788.

…ToType (#4014)" This reverts commit cdca788. Click too fast and the message is messy. Will re-merge.

gesterzhou added 2 commits September 4, 2019 17:04

GEODE-6973: Use cachelistener to synchronize typeToId with IdToType

4b2ae04

Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io> Co-authored-by: Donal Evans <doevans@pivotal.io>

introduce a tmpTypeToId map for listener

8f8dd49

gesterzhou requested review from bschuchardt and upthewaterspout September 6, 2019 23:19

Donal Evans and others added 4 commits September 6, 2019 17:00

Fixed some test failures

1657440

Co-authored-by: Donal Evans <doevans@pivotal.io> Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>

fix junit test

c7464a0

Merge remote-tracking branch 'refs/remotes/donal/feature/GEODE-6973-2…

023dbe7

…' into feature/GEODE-6973-2

enumId should also be put into the tmp map when listener is triggered

c412726

jhuynh1 reviewed Sep 9, 2019

View reviewed changes

bschuchardt previously requested changes Sep 9, 2019

View reviewed changes

gesterzhou added 2 commits September 9, 2019 11:13

consolidate some comments of reviewers

05a97ab

change the name of the local map

94772bd

gesterzhou requested a review from bschuchardt September 9, 2019 18:35

gesterzhou requested a review from jhuynh1 September 9, 2019 18:38

add back load into map in initalize, since it will be fast.

e7e2353

bschuchardt reviewed Sep 9, 2019

View reviewed changes

Added DUnit test

251e552

Co-authored-by: Donal Evans <doevans@pivotal.io> Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>

gesterzhou closed this Sep 9, 2019

gesterzhou reopened this Sep 9, 2019

Spotless apply

6255c37

Authored-by: Donal Evans <doevans@pivotal.io>

Refactoring DUnit tests

c241da4

Authored-by: Donal Evans <doevans@pivotal.io>

upthewaterspout reviewed Sep 9, 2019

View reviewed changes

Removing bad tests

4171749

Authored-by: Donal Evans <doevans@pivotal.io>

refactor the local maps

68b338e

gesterzhou requested review from bschuchardt and upthewaterspout September 10, 2019 05:21

bschuchardt approved these changes Sep 10, 2019

View reviewed changes

Donal Evans added 2 commits September 10, 2019 09:12

Corrected reload from region qualifier

2cdf7f8

Authored-by: Donal Evans <doevans@pivotal.io>

Use method to access PdxTypes region

5be026c

Authored-by: Donal Evans <doevans@pivotal.io>

upthewaterspout approved these changes Sep 10, 2019

View reviewed changes

save() should have a boolean parameter to determine if to save into p…

be71b23

…ending map

gesterzhou merged commit cdca788 into apache:develop Sep 10, 2019

gesterzhou added a commit that referenced this pull request Sep 10, 2019

Revert "GEODE-6973: Use cachelistener to synchronize typeToId with Id…

6bc9249

…ToType (#4014)" This reverts commit cdca788.

gesterzhou added a commit that referenced this pull request Sep 10, 2019

Revert "GEODE-6973: Use cachelistener to synchronize typeToId with Id…

4b9148e

…ToType (#4014)" This reverts commit cdca788. Click too fast and the message is messy. Will re-merge.

gesterzhou deleted the feature/GEODE-6973-2 branch September 11, 2019 00:03

Conversation

gesterzhou commented Sep 5, 2019

For all changes:

Note:

Uh oh!

jhuynh1 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

gesterzhou Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

jhuynh1 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

bschuchardt left a comment

Choose a reason for hiding this comment

Uh oh!

gesterzhou commented Sep 9, 2019

Uh oh!

bschuchardt left a comment

Choose a reason for hiding this comment

Uh oh!

upthewaterspout commented Sep 9, 2019

Uh oh!

upthewaterspout left a comment

Choose a reason for hiding this comment

Uh oh!

bschuchardt commented Sep 9, 2019

Uh oh!

DonalEvans commented Sep 9, 2019

Uh oh!

gesterzhou commented Sep 9, 2019

Uh oh!

gesterzhou commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gesterzhou commented Sep 10, 2019

Uh oh!

bschuchardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gesterzhou commented Sep 10, 2019 •

edited

Loading