Rc realtime #3730

qdpham13 · 2022-05-16T19:07:56Z

Creates Realtime Http client file. Includes ConfigUpdate Listener & Registration classes. Also includes empty helper methods that will be filled out in future PRs.
As part of bug/230368934

…egistration. Also exposes methods to be used by public RC file.

…e-dev

google-oss-bot · 2022-05-16T19:14:40Z

Coverage Report ¹

Affected Products

firebase-config
Overall coverage changed from 88.93% (bd761a7) to 87.49% (86af789) by -1.44%.
Filename Base (bd761a7) Merge (86af789) Diff
ConfigRealtimeHttpClient.java ? 0.00% ?
ConfigUpdateListener.java ? 0.00% ?
ConfigUpdateListenerRegistration.java ? 0.00% ?

Test Logs

Notes

Commit (86af789) is created by Prow via merging PR base commit (bd761a7) and head commit (0bd4c2b).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/7Q9sPcBiAn.html

google-oss-bot · 2022-05-16T19:19:26Z

Size Report ¹

Affected Products

firebase-config
Type Base (bd761a7) Merge (86af789) Diff
aar 63.3 kB 65.1 kB +1.85 kB (+2.9%)
apk (release) 733 kB 733 kB +652 B (+0.1%)

Test Logs

Notes

Commit (86af789) is created by Prow via merging PR base commit (bd761a7) and head commit (0bd4c2b).

https://storage.googleapis.com/firebase-sdk-metric-reports/zQzR6JmFH8.html

qdpham13 · 2022-05-16T20:59:42Z

For check-changes, it looks like all the tests passed but it said it failed. And for copyright-check, I'm not sure how to add the new file to the copyright so leaving it for now.

qdpham13 · 2022-05-17T18:09:27Z

/test smoke-tests

vkryachko

I am not sure why the base branch is not master, not having seen the previous changes to this branch makes it hard to review this PR

vkryachko · 2022-05-18T13:38:28Z

...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java

+  public static class ConfigUpdateListenerRegistration {
+    private final ConfigRealtimeHttpClient client;
+    private final int listenerKey;
+
+    public ConfigUpdateListenerRegistration(ConfigRealtimeHttpClient client, int listenerKey) {
+      this.client = client;
+      this.listenerKey = listenerKey;
+    }
+
+    public void remove() {
+      client.removeRealtimeConfigUpdateListener(listenerKey);
+    }
+  }
+
+  // Event Listener interface to be used by developers.
+  public interface ConfigUpdateListener {
+    // Call back for when Realtime fetches.
+    void onEvent();
+
+    void onError(Exception error);
+  }


Are these types supposed to be used by developers? if so I suggest extracting them as top level classes and putting them under com.google.firebase.remoteconfig

Moved to top level RC class. And the base branch is not master b/c we wanted to not have our new SDK changes be public in future releases while we're not completely finished with implementation. We wanted to wait until full implementation to finish before merging into master. Do you think we should just merge into master instread?

Oh, that's fine, I thought the base branch contained your prototype implementation already. if the base branch is not diverged from master, it's ok to keep this all on a feature branch

…egistration. Also exposes methods to be used by public RC file.

…e-dev

…blic level.

google-oss-bot · 2022-05-18T16:44:39Z

The public api surface has changed for the subproject firebase-config:
error: Added class com.google.firebase.remoteconfig.FirebaseRemoteConfig.ConfigUpdateListener [AddedInterface]
error: Added class com.google.firebase.remoteconfig.FirebaseRemoteConfig.ConfigUpdateListenerRegistration [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

…blic level.

qdpham13 · 2022-05-19T18:10:34Z

/test check-coverage-changed

vkryachko · 2022-05-19T18:33:23Z

...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java

+
+public class ConfigRealtimeHttpClient {
+
+  private final Map<Integer, ConfigUpdateListener> listeners;


Why do you need a map? wouldn't a Set<ConfigUpdateListener> work?

Also, looks like you're not synchronizing access to this map/set, this can lead to race conditions

Changed to synchronized set

vkryachko · 2022-05-19T18:47:15Z

...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java

+      ConfigUpdateListener configUpdateListener) {
+    listeners.put(listenerCount, configUpdateListener);
+    beginRealtime();
+    return new ConfigUpdateListenerRegistrationInternal(this, listenerCount++);


Instead of capturing this explicitly, you can make ConfigUpdateListenerRegistrationInternal non static.

Update to non static

vkryachko · 2022-05-19T18:47:50Z

...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java

+    return new ConfigUpdateListenerRegistrationInternal(this, listenerCount++);
+  }
+
+  public void removeRealtimeConfigUpdateListener(int listenerKey) {


Left public so registration could access but fixed with above change. Changed to private.

vkryachko

Unapprove for now

Unapprove

Make internal ConfigUpdateListenerRegistration non-static to access private listener removal method from outer class. Add synchonization to listeners set.

vkryachko · 2022-05-19T20:02:20Z

...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java

+  private final Set<ConfigUpdateListener> listeners;
+
+  public ConfigRealtimeHttpClient() {
+    listeners = Collections.synchronizedSet(new LinkedHashSet<ConfigUpdateListener>());


I suggest external synchronization instead of synchronizedSet as it's still not safe to iterate over synchronizedSet without explicit synchronization, which you will need when you implement event delivery.

So I would suggest making all methods that mutate the set synchronized and add a @GuardedBy("this") annotation to the listeners field.

That makes sense, thanks for the suggestions! I'll make sure to keep this mind for other methods that interact with it.

google-oss-bot · 2022-05-20T20:37:30Z

@qdpham13: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
build-plugins-check	`99c166c`	link	`/test build-plugins-check`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

qdpham13 · 2022-05-20T20:40:33Z

/test copyright-check

qdpham13 added 4 commits May 13, 2022 20:32

Creates Realtime Http client file. Includes ConfigUpdate Listener & R…

488e8e7

…egistration. Also exposes methods to be used by public RC file.

Creates Realtime Http client file. Includes ConfigUpdate Listener & R…

fc01819

…egistration. Also exposes methods to be used by public RC file.

Merge remote-tracking branch 'origin/rc-realtime-dev' into rc-realtim…

41898e7

…e-dev

Update stream name

a00ce0c

google-oss-bot added the size/M label May 16, 2022

Add copyright info to top of file.

31af37a

Update comments

187864b

qdpham13 requested review from vkryachko and rlazo May 16, 2022 21:14

Format file.

a9d3a97

vkryachko reviewed May 18, 2022

View reviewed changes

qdpham13 added 6 commits May 18, 2022 16:22

Creates Realtime Http client file. Includes ConfigUpdate Listener & R…

3642424

…egistration. Also exposes methods to be used by public RC file.

Update stream name

19956a8

Add copyright info to top of file.

93512af

Update comments

fa56069

Format file.

50da87e

Merge remote-tracking branch 'origin/rc-realtime-dev' into rc-realtim…

99c166c

…e-dev

google-oss-bot added size/XXL and removed size/M labels May 18, 2022

qdpham13 changed the base branch from realtime-rc-merge to master May 18, 2022 16:24

google-oss-bot added size/M and removed size/XXL labels May 18, 2022

qdpham13 changed the base branch from master to realtime-rc-merge May 18, 2022 16:25

Moved ConfigUpdateListener and ConfigUpdateListenerRegistration to pu…

567aa14

…blic level.

Moved ConfigUpdateListener and ConfigUpdateListenerRegistration to pu…

4c16e92

…blic level.

google-oss-bot added size/XXL and removed size/L labels May 19, 2022

qdpham13 changed the base branch from realtime-rc-merge to master May 19, 2022 17:32

qdpham13 changed the base branch from master to realtime-rc-merge May 19, 2022 17:32

qdpham13 changed the base branch from realtime-rc-merge to master May 19, 2022 17:32

qdpham13 changed the base branch from master to realtime-rc-merge May 19, 2022 17:35

qdpham13 changed the base branch from realtime-rc-merge to master May 19, 2022 17:35

Merge remote-tracking branch 'origin/master' into rc-realtime-dev

4eca1dc

qdpham13 changed the base branch from master to realtime-rc-merge May 19, 2022 17:36

google-oss-bot added size/L and removed size/XXL labels May 19, 2022

Reupdate files.

32e980a

vkryachko approved these changes May 19, 2022

View reviewed changes

vkryachko self-requested a review May 19, 2022 18:48

vkryachko previously requested changes May 19, 2022

View reviewed changes

Make changes based on PR comments;

527ae0c

Make internal ConfigUpdateListenerRegistration non-static to access private listener removal method from outer class. Add synchonization to listeners set.

qdpham13 requested a review from vkryachko May 19, 2022 19:41

vkryachko reviewed May 19, 2022

View reviewed changes

Remove synchronized set and add explicit synchonized to methods.

a6f69c2

vkryachko approved these changes May 19, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into rc-realtime-dev

a22e5d3

Merge remote-tracking branch 'origin/master' into rc-realtime-dev

0bd4c2b

qdpham13 merged commit 465488e into realtime-rc-merge May 23, 2022

qdpham13 deleted the rc-realtime-dev branch May 23, 2022 23:19

qdpham13 restored the rc-realtime-dev branch May 23, 2022 23:19

firebase locked and limited conversation to collaborators Jun 23, 2022


		public class ConfigRealtimeHttpClient {

		private final Map<Integer, ConfigUpdateListener> listeners;

Rc realtime #3730

Rc realtime #3730

Uh oh!

Conversation

qdpham13 commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-config

Test Logs

Notes

Uh oh!

google-oss-bot commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-config

Test Logs

Notes

Uh oh!

qdpham13 commented May 16, 2022

Uh oh!

qdpham13 commented May 17, 2022

Uh oh!

vkryachko left a comment

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!

google-oss-bot commented May 18, 2022

Uh oh!

qdpham13 commented May 19, 2022

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!

vkryachko left a comment

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!

google-oss-bot commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qdpham13 commented May 20, 2022

Uh oh!

Uh oh!

qdpham13 commented May 16, 2022 •

edited

Loading

google-oss-bot commented May 16, 2022 •

edited

Loading

Coverage Report ¹

`firebase-config`

google-oss-bot commented May 16, 2022 •

edited

Loading

Size Report ¹

`firebase-config`

google-oss-bot commented May 20, 2022 •

edited

Loading