feat(apollo-client): add method interestedChangedKeys to ConfigChangeEvent by Anilople · Pull Request #3666 · apolloconfig/apollo

Anilople · 2021-05-08T11:52:42Z

What's the purpose of this PR

Let user can get interestedChangedKeys through ConfigChangeEvent

Which issue(s) this PR fixes:

Fixes #3664

Brief changelog

resolve interestedChangedKeys in class AbstractConfig

Follow this checklist to help us incorporate your contribution quickly and easily:

Read the Contributing Guide before making this pull request.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit tests to verify the code.
Run mvn clean test to make sure this pull request doesn't break anything.

…Event

Anilople · 2021-05-08T11:54:27Z

          Transaction transaction = Tracer.newTransaction("Apollo.ConfigChangeListener", listenerName);
          try {
-            listener.onChange(changeEvent);
+            listener.onChange(resolve(listener, changeEvent));


How about generate a new ConfigChangeEvent here?

Subclass's ConfigChangeEvent + resolved interestedChangedKeys ==> new ConfigChangeEvent

That will change the default behavior of subclass, but still keep compatible.

How about we create a subclass of ConfigChangeEvent, which contains the original config change event and the interested keys so that we don't need to create a new instance?

When subclasses of AbstractConfig invoke method com.ctrip.framework.apollo.internals.AbstractConfig#fireConfigChange, they don't know about any information of interestedKeys because interestedKeys are saved in AbstractConfig.

If we create a subclass of ConfigChangeEvent, when method com.ctrip.framework.apollo.internals.AbstractConfig#fireConfigChange is invoking, we still need to create a new instance of subclass of ConfigChangeEvent, because the listener need to get this instance?

you are right, I was thinking we may use the composition pattern to avoid create a lot of ConfigChangeEvent instances.

Change ConfigChangeEvent to an abstract class?

public abstract class ConfigChangeEvent { // content }

Or create a new class AbstractConfigChangeEvent,

public abstract class AbstractConfigChangeEvent { public abstract Set<String> changedKeys(); public abstract Set<String> interestedChangedKeys(); public abstract ConfigChange getChange(String key); public abstract boolean isChanged(String key); public abstract String getNamespace(); }

then let ConfigChangeEvent extends it?

public class ConfigChangeEvent extends AbstractConfigChangeEvent { // ... }

My original idea was to create an abstract class or an interface, and then use composition to wrap the original ConfigChangeEvent into InterestedConfigChangeEvent. But the current implementation also looks good to me.

codecov-commenter · 2021-05-08T12:41:31Z

Codecov Report

Merging #3666 (479a5d2) into master (ff38fe0) will increase coverage by 0.07%.
The diff coverage is 89.65%.

@@             Coverage Diff              @@
##             master    #3666      +/-   ##
============================================
+ Coverage     50.32%   50.40%   +0.07%     
- Complexity     2374     2389      +15     
============================================
  Files           457      458       +1     
  Lines         14435    14472      +37     
  Branches       1457     1467      +10     
============================================
+ Hits           7264     7294      +30     
- Misses         6665     6674       +9     
+ Partials        506      504       -2

Impacted Files	Coverage Δ
...trip/framework/apollo/model/ConfigChangeEvent.java	`88.88% <66.66%> (-11.12%)`	⬇️
...rip/framework/apollo/internals/AbstractConfig.java	`83.15% <89.79%> (+4.50%)`	⬆️
...trip/framework/apollo/internals/DefaultConfig.java	`82.97% <100.00%> (ø)`
.../apollo/internals/InterestedConfigChangeEvent.java	`100.00% <100.00%> (ø)`
...ctrip/framework/apollo/internals/SimpleConfig.java	`86.84% <100.00%> (ø)`
...work/apollo/biz/message/DatabaseMessageSender.java	`50.00% <0.00%> (-14.59%)`	⬇️
...framework/apollo/openapi/entity/ConsumerAudit.java	`42.42% <0.00%> (-6.07%)`	⬇️
...rk/apollo/spring/property/SpringValueRegistry.java	`83.33% <0.00%> (-5.56%)`	⬇️
...mework/apollo/openapi/service/ConsumerService.java	`47.57% <0.00%> (-1.95%)`	⬇️
.../framework/apollo/spring/property/SpringValue.java	`87.71% <0.00%> (-1.76%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff38fe0...479a5d2. Read the comment docs.

nobodyiam

Thanks for the effort, basically I think it's fine. Please find some comments below.
BTW, I think we need to add some unit test for the new apis.

nobodyiam · 2021-05-09T01:13:31Z

          Transaction transaction = Tracer.newTransaction("Apollo.ConfigChangeListener", listenerName);
          try {
-            listener.onChange(changeEvent);
+            listener.onChange(resolve(listener, changeEvent));


How about we create a subclass of ConfigChangeEvent, which contains the original config change event and the interested keys so that we don't need to create a new instance?

github-actions · 2021-05-15T13:04:12Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

nobodyiam · 2021-06-06T06:39:04Z

+
+    UnsupportedOperationConfig config = new UnsupportedOperationConfig();
+    config.addChangeListener(configChangeListener, Collections.singleton("key-nothing"), Collections.singleton(keyPrefix));
+    config.fireConfigChange(namespace, Collections.singletonMap(key,


I would suggest we add some irrelavent change here to check whether the interested keys logic works, e.g.

Map<String, ConfigChange> changes = new HashMap<>(); changes.put(key, new ConfigChange(namespace, key, "123", "456", PropertyChangeType.MODIFIED)); changes.put(anotherKey, new ConfigChange(namespace, anotherKey, null, "someValue", PropertyChangeType.ADDED)); config.fireConfigChange(namespace, changes);

and then verify the changedKeys and interestedChangedKeys methods both work

ConfigChangeListener configChangeListener = spy(new ConfigChangeListener() { @Override public void onChange(ConfigChangeEvent changeEvent) { assertEquals(namespace, changeEvent.getNamespace()); assertEquals(2, changeEvent.changedKeys().size()); assertTrue(changeEvent.changedKeys().containsAll(Sets.newHashSet(key, anotherKey))); assertEquals(1, changeEvent.interestedChangedKeys().size()); assertTrue(changeEvent.interestedChangedKeys().contains(key)); onChangeFuture.set(changeEvent); } });

JaredTan95 · 2021-06-06T07:24:18Z

as #3747 merged, please update changs log. :-)

…Anilople/apollo into feature/interested/key/20210508

nobodyiam

LGTM

nobodyiam

after checking the ci failure, I think there is a bug here.
as the following screenshot shows, the ConfigChangeListener is triggered in 2 for loops, which means it is triggered multiple times.

…kage

Anilople · 2021-06-09T14:01:28Z

+   *
+   * @return how many listener be fired
+   */
+  protected int fireConfigChange(final ConfigChangeEvent changeEvent) {


Will it break the compability when change return type from void to int?

I think it will, since this method is protected, users may have overridden it

nobodyiam · 2021-06-10T00:12:33Z

+    changes.put("key1", new ConfigChange(namespace, "key1", null, "new-value", PropertyChangeType.ADDED));
+    ConfigChangeEvent configChangeEvent = new ConfigChangeEvent(namespace, changes);
+
+    assertEquals(0, abstractConfig.fireConfigChange(configChangeEvent));


I think we can add a counter in this onChange method to test the times of invocation instead of changing the signature

nobodyiam

LGTM

Anilople added 2 commits May 8, 2021 19:49

feat(apollo-client): add method interestedChangedKeys to ConfigChange…

817c9a7

…Event

docs: add Apache LICENSE-2.0

2b28a27

Anilople added the enhancement label May 8, 2021

Anilople commented May 8, 2021

View reviewed changes

fix(apollo-client): null exception

1e40d80

nobodyiam reviewed May 9, 2021

View reviewed changes

Merge branch 'master' into feature/interested/key/20210508

2dda71b

nobodyiam added this to the 1.9.0 milestone May 23, 2021

Anilople and others added 6 commits June 5, 2021 09:26

Merge branch 'master' into feature/interested/key/20210508

67d49bc

delete getChanges in ConfigChangeEvent

835823d

docs: update LICENSE

e6c20a0

feat: use InterestedConfigChangeEvent

66d59ee

revert a test back

cca7e5b

test: interested key config change event

0ce9957

nobodyiam reviewed Jun 6, 2021

View reviewed changes

Anilople and others added 4 commits June 7, 2021 18:30

Merge branch 'master' into feature/interested/key/20210508

428f461

test: add another key

a3a5cfa

Merge branch 'feature/interested/key/20210508' of https://github.com/…

c475174

…Anilople/apollo into feature/interested/key/20210508

Update CHANGES.md

2880a4f

nobodyiam approved these changes Jun 9, 2021

View reviewed changes

nobodyiam self-requested a review June 9, 2021 00:30

nobodyiam requested changes Jun 9, 2021

View reviewed changes

Anilople and others added 5 commits June 9, 2021 20:08

Merge branch 'master' into feature/interested/key/20210508

3064aaf

feat: fireConfigChange return how many listener be fired

e78a706

test: expose bug in fireConfigChange

259cda1

fix: notify multiple time if exists multiple listener

cd30d53

feat: move InterestedConfigChangeEvent, it cannot be use by other pac…

5759735

…kage

Anilople commented Jun 9, 2021

View reviewed changes

Comment thread ...o-client/src/main/java/com/ctrip/framework/apollo/internals/InterestedConfigChangeEvent.java

Anilople requested a review from nobodyiam June 9, 2021 14:06

nobodyiam reviewed Jun 10, 2021

View reviewed changes

rollback fireConfigChange return type to 'void'

479a5d2

Anilople requested a review from nobodyiam June 10, 2021 12:11

nobodyiam approved these changes Jun 11, 2021

View reviewed changes

nobodyiam merged commit 44cf586 into apolloconfig:master Jun 11, 2021

github-actions Bot locked and limited conversation to collaborators Jun 11, 2021

Uh oh!

Conversation

Anilople commented May 8, 2021

What's the purpose of this PR

Which issue(s) this PR fixes:

Brief changelog

Uh oh!

Anilople May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nobodyiam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaredTan95 commented Jun 6, 2021

Uh oh!

nobodyiam left a comment

Choose a reason for hiding this comment

Uh oh!

nobodyiam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nobodyiam Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nobodyiam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Anilople May 8, 2021 •

edited

Loading

codecov-commenter commented May 8, 2021 •

edited

Loading

github-actions Bot commented May 15, 2021 •

edited

Loading

nobodyiam Jun 10, 2021 •

edited

Loading