Skip to content

Realtime Fetch ID #4328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Dec 1, 2022
Merged

Realtime Fetch ID #4328

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6479e73
Create overloaded fetch method specifically for realtime and add cust…
qdpham13 Nov 15, 2022
25e7e2b
Add comments for vars
qdpham13 Nov 16, 2022
2e7fe91
Update header name
qdpham13 Nov 17, 2022
590f9ad
Merge branch 'realtime-rc-merge' into realtime-rc-fetch-id
qdpham13 Nov 17, 2022
15389c8
Change realtimeFetch name and params
qdpham13 Nov 22, 2022
fe13390
Remove fetchHandler tests and move them to seperate PR
qdpham13 Nov 22, 2022
64798e9
Remove unused vars and revert tests
qdpham13 Nov 22, 2022
83d5f4c
Format tests
qdpham13 Nov 22, 2022
7d782ac
Add comments
qdpham13 Nov 22, 2022
1595a03
Format files
qdpham13 Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigAutoFetch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,8 @@ public void run() {

@VisibleForTesting
public synchronized void fetchLatestConfig(int remainingAttempts, long targetVersion) {
Task<ConfigFetchHandler.FetchResponse> fetchTask = configFetchHandler.fetch(0L);
Task<ConfigFetchHandler.FetchResponse> fetchTask =
configFetchHandler.realtimeFetch(0L, FETCH_RETRY - remainingAttempts + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use a comment since the math is a bit cryptic on its own. Maybe even worth just keeping a separate attempts counter?

Slightly related, FETCH_RETRIES_ALLOWED or INITIAL_FETCH_RETRIES would be more descriptive as a variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

fetchTask.onSuccessTask(
(fetchResponse) -> {
long newTemplateVersion = 0;
Expand Down
84 changes: 77 additions & 7 deletions ...se-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigFetchHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ public class ConfigFetchHandler {
*/
@VisibleForTesting static final String FIRST_OPEN_TIME_KEY = "_fot";

/** Custom Http header key to identify the fetch type. */
private static final String X_FIREBASE_RC_FETCH_TYPE = "X-Firebase-RC-Fetch-Type";
/** Fetch identifier for Realtime. */
private static final String REALTIME_FETCH_TYPE = "Realtime";
/** Fetch identifier for Base. */
private static final String BASE_FETCH_TYPE = "Base";

private final FirebaseInstallationsApi firebaseInstallations;
private final Provider<AnalyticsConnector> analyticsConnector;

Expand Down Expand Up @@ -156,13 +163,67 @@ public Task<FetchResponse> fetch() {
* updates, the {@link FetchResponse}'s configs will be {@code null}.
*/
public Task<FetchResponse> fetch(long minimumFetchIntervalInSeconds) {

// Make a copy to prevent any concurrency issues between Fetches.
Map<String, String> copyOfCustomHttpHeaders = new HashMap<>(customHttpHeaders);
copyOfCustomHttpHeaders.put(X_FIREBASE_RC_FETCH_TYPE, BASE_FETCH_TYPE + "/" + 1);

return fetchedConfigsCache
.get()
.continueWithTask(
executor,
(cachedFetchConfigsTask) ->
fetchIfCacheExpiredAndNotThrottled(
cachedFetchConfigsTask,
minimumFetchIntervalInSeconds,
copyOfCustomHttpHeaders));
}

/**
* Starts fetching configs from the Firebase Remote Config server.
*
* <p>Guarantees consistency between memory and disk; fetched configs are saved to memory only
* after they have been written to disk.
*
* <p>Fetches even if the read of the fetch cache fails (assumes there are no cached fetched
* configs in that case).
*
* <p>If the fetch request could not be created or there was error connecting to the server, the
* returned Task throws a {@link FirebaseRemoteConfigClientException}.
*
* <p>If the server responds with an error, the returned Task throws a {@link
* FirebaseRemoteConfigServerException}.
*
* <p>If any of the following is true, then the returned Task throws a {@link
* FirebaseRemoteConfigFetchThrottledException}:
*
* <ul>
* <li>The backoff duration from a previous throttled exception has not expired,
* <li>The backend responded with a throttled error, or
* <li>The backend responded with unavailable errors for the last two fetch requests.
* </ul>
*
* @return A {@link Task} representing a Realtime fetch call that returns a {@link FetchResponse}
* with the configs fetched from the backend. If the backend was not called or the backend had
* no updates, the {@link FetchResponse}'s configs will be {@code null}.
*/
public Task<FetchResponse> realtimeFetch(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit off to me that half the header comes from calling a particular method (realtimeFetch) and half from a param passed the method (fetchAttemptNumber). It also breaks the encapsulation of fetch within realtime to have realtime implementation details here.

Since the type and attempt are fetch details and realtime is just making use of them, passing both in (i.e. fetchWithMetadata(long minimumFetchIntervalInSeconds, FetchType fetchType, int fetchAttempt)) could make sense. A FetchType enum could make sure the value passed in makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this makes sense. I updated it to create a new method that isn't so closely tied to Realtime.

long minimumFetchIntervalInSeconds, int fetchAttemptNumber) {

// Make a copy to prevent any concurrency issues between Fetches.
Map<String, String> copyOfCustomHttpHeaders = new HashMap<>(customHttpHeaders);
copyOfCustomHttpHeaders.put(
X_FIREBASE_RC_FETCH_TYPE, REALTIME_FETCH_TYPE + "/" + fetchAttemptNumber);

return fetchedConfigsCache
.get()
.continueWithTask(
executor,
(cachedFetchConfigsTask) ->
fetchIfCacheExpiredAndNotThrottled(
cachedFetchConfigsTask, minimumFetchIntervalInSeconds));
cachedFetchConfigsTask,
minimumFetchIntervalInSeconds,
copyOfCustomHttpHeaders));
}

/**
Expand All @@ -173,7 +234,9 @@ public Task<FetchResponse> fetch(long minimumFetchIntervalInSeconds) {
* fetch time and {@link BackoffMetadata} in {@link ConfigMetadataClient}.
*/
private Task<FetchResponse> fetchIfCacheExpiredAndNotThrottled(
Task<ConfigContainer> cachedFetchConfigsTask, long minimumFetchIntervalInSeconds) {
Task<ConfigContainer> cachedFetchConfigsTask,
long minimumFetchIntervalInSeconds,
Map<String, String> customFetchHeaders) {
Date currentTime = new Date(clock.currentTimeMillis());
if (cachedFetchConfigsTask.isSuccessful()
&& areCachedFetchConfigsValid(minimumFetchIntervalInSeconds, currentTime)) {
Expand Down Expand Up @@ -218,7 +281,7 @@ && areCachedFetchConfigsValid(minimumFetchIntervalInSeconds, currentTime)) {
String installationId = installationIdTask.getResult();
String installationToken = installationAuthTokenTask.getResult().getToken();
return fetchFromBackendAndCacheResponse(
installationId, installationToken, currentTime);
installationId, installationToken, currentTime, customFetchHeaders);
});
}

Expand Down Expand Up @@ -278,9 +341,13 @@ private String createThrottledMessage(long throttledDurationInMillis) {
* {@code fetchedConfigsCache}.
*/
private Task<FetchResponse> fetchFromBackendAndCacheResponse(
String installationId, String installationToken, Date fetchTime) {
String installationId,
String installationToken,
Date fetchTime,
Map<String, String> customFetchHeaders) {
try {
FetchResponse fetchResponse = fetchFromBackend(installationId, installationToken, fetchTime);
FetchResponse fetchResponse =
fetchFromBackend(installationId, installationToken, fetchTime, customFetchHeaders);
if (fetchResponse.getStatus() != Status.BACKEND_UPDATES_FETCHED) {
return Tasks.forResult(fetchResponse);
}
Expand All @@ -303,7 +370,10 @@ private Task<FetchResponse> fetchFromBackendAndCacheResponse(
*/
@WorkerThread
private FetchResponse fetchFromBackend(
String installationId, String installationToken, Date currentTime)
String installationId,
String installationToken,
Date currentTime,
Map<String, String> customFetchHeaders)
throws FirebaseRemoteConfigException {
try {
HttpURLConnection urlConnection = frcBackendApiClient.createHttpURLConnection();
Expand All @@ -315,7 +385,7 @@ private FetchResponse fetchFromBackend(
installationToken,
getUserProperties(),
frcMetadata.getLastFetchETag(),
customHttpHeaders,
customFetchHeaders,
getFirstOpenTime(),
currentTime);

Expand Down
17 changes: 11 additions & 6 deletions firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,7 +1156,8 @@ public void realtime_stream_listen_and_retry_success() throws Exception {
new ByteArrayInputStream(
"{ \"latestTemplateVersionNumber\": 1 }".getBytes(StandardCharsets.UTF_8)));
when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L);
when(mockFetchHandler.fetch(0)).thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
when(mockFetchHandler.realtimeFetch(0, 1))
.thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
configAutoFetch.listenForNotifications();

verify(mockRetryListener).onEvent();
Expand Down Expand Up @@ -1245,7 +1246,8 @@ public void realtime_stream_listen_and_failsafe_disabled() throws Exception {
"{ \"featureDisabled\": false, \"latestTemplateVersionNumber\": 2 }"
.getBytes(StandardCharsets.UTF_8)));
when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L);
when(mockFetchHandler.fetch(0)).thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
when(mockFetchHandler.realtimeFetch(0, 1))
.thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
configAutoFetch.listenForNotifications();

verify(mockUnavailableEventListener, never())
Expand All @@ -1258,7 +1260,8 @@ public void realtime_stream_listen_get_inputstream_fail() throws Exception {
when(mockHttpURLConnection.getResponseCode()).thenReturn(200);
when(mockHttpURLConnection.getInputStream()).thenThrow(IOException.class);
when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L);
when(mockFetchHandler.fetch(0)).thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
when(mockFetchHandler.realtimeFetch(0, 1))
.thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
configAutoFetch.listenForNotifications();

verify(mockInvalidMessageEventListener).onError(any(FirebaseRemoteConfigClientException.class));
Expand All @@ -1267,16 +1270,18 @@ public void realtime_stream_listen_get_inputstream_fail() throws Exception {
@Test
public void realtime_stream_autofetch_success() {
when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L);
when(mockFetchHandler.fetch(0)).thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
configAutoFetch.fetchLatestConfig(1, 1);
when(mockFetchHandler.realtimeFetch(0, 1))
.thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
configAutoFetch.fetchLatestConfig(3, 1);

verify(mockOnEventListener).onEvent();
}

@Test
public void realtime_stream_autofetch_failure() {
when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L);
when(mockFetchHandler.fetch(0)).thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
when(mockFetchHandler.realtimeFetch(0, 3))
.thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
configAutoFetch.fetchLatestConfig(1, 1000);

verify(mockNotFetchedEventListener).onError(any(FirebaseRemoteConfigServerException.class));
Expand Down
Loading