Add SessionMaintainerFollower and create in FirebaseSessions when…
#5368
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Per [b/302184829](https://b.corp.google.com/issues/302184829), This configures the newly added [post release cleanup workflow](https://github.com/firebase/firebase-android-sdk/blob/master/.github/workflows/post_release_cleanup.yml) to add `NO_RELEASE_CHANGE` to the PR's body by default.
… we're a follower process.
added 2 commits
September 26, 2023 23:09
Coverage Report 1This report is too large (214,739 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
Size Report 1Affected Products
Test Logs |
mrober
reviewed
Sep 27, 2023
|
|
||
| override val isForegroundProcess: Boolean | ||
| get() { | ||
| val runningAppProcessInfo = RunningAppProcessInfo() | ||
| ActivityManager.getMyMemoryState(runningAppProcessInfo) | ||
| return runningAppProcessInfo.importance == RunningAppProcessInfo.IMPORTANCE_FOREGROUND | ||
| return runningAppProcessInfo.importance <= RunningAppProcessInfo.IMPORTANCE_FOREGROUND_SERVICE |
There was a problem hiding this comment.
I think this should go back to runningAppProcessInfo.importance == RunningAppProcessInfo.IMPORTANCE_FOREGROUND because IMPORTANCE_FOREGROUND_SERVICE is only available after a specific api level, and it'll make our logic complicated to also count services as leaders.
There was a problem hiding this comment.
the one integration test fails if we don't consider foreground services to be foreground processes https://github.com/firebase/firebase-android-sdk/blob/master/firebase-sessions/test-app/src/androidTest/kotlin/com/google/firebase/testing/sessions/FirebaseSessionsTest.kt
That suggests to me that we may need it. Thoughts?
mrober
reviewed
Sep 27, 2023
| SessionMaintainer { | ||
| val tag = "SessionMaintainerFollow" | ||
|
|
||
| override fun register(subscriber: SessionSubscriber) { |
There was a problem hiding this comment.
You can say = Unit instead of an empty body. Looks better in my opinion.
There was a problem hiding this comment.
Is a no-op the right thing to do for followers? Should they still notify the subscribers on register?
Perf relies on the AQS session id being set before the call to firebaseSessions.register returns. Crashlytics is ok without an AQS session id.
There was a problem hiding this comment.
Okay. We can force a lookup.
There was a problem hiding this comment.
actually, I'm unsure. We notify subscribers on init, and getRegisteredSusbscribers claims that it blocks until all subscribers are registered. Seems this should be okay, no?
mrober
reviewed
Sep 27, 2023
| Log.e(tag, "Error reading stored session data.", exception) | ||
| emit(emptyPreferences()) | ||
| } | ||
| .map { preferences -> mapSessionsData(preferences) } |
There was a problem hiding this comment.
Can this be something like .map(::mapSessionsData) ?
There was a problem hiding this comment.
that didn't work
mrober
reviewed
Sep 27, 2023
| FirebaseSessionsData(preferences[FirebaseSessionDataKeys.SESSION_ID]) | ||
| } | ||
|
|
||
| const val SESSION_CONFIGS_NAME = "firebase_session_settings" |
There was a problem hiding this comment.
Putting top level constants directly in the file will cause the Firebase API check to complain, because it will generate a public class to put this in for Java. Maybe this and the FirebaseSessionDataKeys can go in a companion object instead.
mrober
reviewed
Sep 27, 2023
| @@ -22,4 +22,6 @@ import com.google.firebase.sessions.ProcessDetails | |||
| class FakeProcessDetails( | |||
| override val isDefaultProcess: Boolean = true, | |||
There was a problem hiding this comment.
If we have the process name and default process name in the interface, do we need this isDefaultProcess anymore?
mrober
reviewed
Sep 27, 2023
| data class FirebaseSessionsData(val sessionId: String?) | ||
|
|
||
| /** Persists session data that needs to be synchronized across processes */ | ||
| class SessionsDataRepository(private val context: Context) { |
There was a problem hiding this comment.
These classes should all be internal visibility.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… we're a follower process.