Realtime Fetch ID #4328
Realtime Fetch ID #4328
Conversation
Generated by 🚫 Danger |
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
qdpham13
changed the title
| @@ -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); | |||
There was a problem hiding this comment.
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.
| * 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess this makes sense. I updated it to create a new method that isn't so closely tied to Realtime.
| @@ -337,6 +337,22 @@ public void fetchWithExpiration_cacheHasNotExpired_doesNotFetchFromBackend() thr | |||
| verifyBackendIsNeverCalled(); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Nice realtime tests! 😄 Mind splitting the ones that aren't for the fetch id into their own PR to make this a bit easier to review? Also, I'm not sure I see a test for the header being set?
There was a problem hiding this comment.
Removed and will move to a separate PR
| * @return A {@link Task} representing an immediate 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}. | ||
| * FetchType and fetchAttemptNumber help detail what started the fetch call. |
There was a problem hiding this comment.
This line would make more sense in a @params comment rather than @return
| @@ -626,4 +691,14 @@ public ConfigContainer getFetchedConfigs() { | |||
| int LOCAL_STORAGE_USED = 2; | |||
| } | |||
| } | |||
|
|
|||
| public enum FetchType { | |||
| BASE, | |||
There was a problem hiding this comment.
The enum value can be stored in an instance field, which is more clear to read and makes it independent of the enum itself, i.e.
BASE("Base"),
...
private final int value;
FetchType(value) {
this.value = value;
}
String getValue() {
return value;
}
* Create overloaded fetch method specifically for realtime and add custom headers * Add comments for vars * Update header name * Change realtimeFetch name and params * Remove fetchHandler tests and move them to seperate PR * Remove unused vars and revert tests * Format tests * Add comments * Format files
Implementation of Realtime Fetch ID: go/realtime-rc-fetch-id