feat(apollo-client): add method interestedChangedKeys to ConfigChangeEvent #3666

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

apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java

@@ -450,7 +468,7 @@ public void run() {
          String listenerName = listener.getClass().getName();
          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

apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java

@@ -450,7 +468,7 @@ public void run() {
          String listenerName = listener.getClass().getName();
          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?

apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java

apollo-client/src/main/java/com/ctrip/framework/apollo/model/ConfigChangeEvent.java

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

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

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

...o-client/src/test/java/com/ctrip/framework/apollo/model/InterestedConfigChangeEventTest.java

+
+    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

apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java

+   *
+   * @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

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

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

apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java

+    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

...o-client/src/main/java/com/ctrip/framework/apollo/internals/InterestedConfigChangeEvent.java Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apollo-client): add method interestedChangedKeys to ConfigChangeEvent #3666

feat(apollo-client): add method interestedChangedKeys to ConfigChangeEvent #3666

Anilople commented May 8, 2021

Anilople May 8, 2021 •

edited

Loading

nobodyiam May 9, 2021

Anilople Jun 5, 2021

nobodyiam Jun 6, 2021

Anilople Jun 7, 2021

nobodyiam Jun 9, 2021

codecov-commenter commented May 8, 2021 •

edited

Loading

nobodyiam left a comment

nobodyiam May 9, 2021

github-actions bot commented May 15, 2021 •

edited

Loading

nobodyiam Jun 6, 2021

JaredTan95 commented Jun 6, 2021

nobodyiam left a comment

nobodyiam left a comment

Anilople Jun 9, 2021

nobodyiam Jun 10, 2021 •

edited

Loading

nobodyiam Jun 10, 2021

nobodyiam left a comment

feat(apollo-client): add method interestedChangedKeys to ConfigChangeEvent #3666

feat(apollo-client): add method interestedChangedKeys to ConfigChangeEvent #3666

Conversation

Anilople commented May 8, 2021

What's the purpose of this PR

Which issue(s) this PR fixes:

Brief changelog

Anilople May 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 8, 2021 • edited Loading

Codecov Report

nobodyiam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 15, 2021 • edited Loading

Choose a reason for hiding this comment

JaredTan95 commented Jun 6, 2021

nobodyiam left a comment

Choose a reason for hiding this comment

nobodyiam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nobodyiam Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nobodyiam left a comment

Choose a reason for hiding this comment

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