Dropping Support for App Engine Java 7 Runtime #153
Conversation
CHANGELOG.md
Outdated
| - [changed] Removed the deprecated `FirebaseCredential` interface. | ||
| - [changed] Removed the deprecated `Task` interface along with the | ||
| `com.google.firebase.tasks` package. | ||
| - [changed] Dropped support for App Engine Java 7 runtime. |
There was a problem hiding this comment.
This might sound scary to someone unfamiliar with the subject. Do you want to state that Java 8 is now fully supported? Maybe something like Dropped support for App Engine's Java 7 runtime. We recommend to use the Admin SDK with Java 8.
| List<String> loggedComponents; | ||
| Logger.Level logLevel = Logger.Level.INFO; | ||
| boolean persistenceEnabled; | ||
| long cacheSize = DEFAULT_CACHE_SIZE; |
There was a problem hiding this comment.
Can any of these be private?
There was a problem hiding this comment.
All of them are accessed from DatabaseConfig and a couple of other places. I managed to make firebaseApp package-protected though.
|
|
||
| public FirebaseScheduledExecutor( | ||
| ThreadFactory threadFactory, String name, Thread.UncaughtExceptionHandler handler) { | ||
| super(1, decorateThreadFactory(threadFactory, name, handler)); |
There was a problem hiding this comment.
I think it would be super duper awesome if we just used ScheduledThreadPoolExecutor in our code. It would be much easier to grasp that we are now using an off-the-shelf executor. Maybe you can find a different place for decorateThreadFactory?
There was a problem hiding this comment.
In addition to decorating the ThreadFactory this class also customizes the executor a little bit by calling setRemoveOnCancelPolicy(). I'd rather not duplicate that in all the places we use this class. Also having all this in one place gives us an easy way to introduce more customizations in the future if required. WDYT?
There was a problem hiding this comment.
Another way to get around this would be to introduce a new util method somewhere:
public static ScheduledThreadPoolExecutor newScheduledExecutor(ThreadFactory factory, String name)
We can do all the decorating/customizing in the method. DefaultRunLoop will have to find another approach though, since it extends the executor. Thoughts?
There was a problem hiding this comment.
I like your last approach better, but I can't think of a good way to replace the inheritance in DefaultRunLoop. We might want to ponder a little more about this, but I don't think we should hold up this PR on it. If anything, we could argue that this derisks this change since it keeps the existing pattern.
| setRemoveOnCancelPolicy(true); | ||
| } | ||
|
|
||
| static ThreadFactory decorateThreadFactory(ThreadFactory threadFactory, String name) { |
There was a problem hiding this comment.
The name of this external method is somewhat generic and as such it is hard to tell what it does. Do you mind finding a better name?
There was a problem hiding this comment.
Changed to getThreadFactoryWithName()
|
@schmidt-sebastian I've addressed the trivial comments. As for removing |
|
|
||
| public FirebaseScheduledExecutor( | ||
| ThreadFactory threadFactory, String name, Thread.UncaughtExceptionHandler handler) { | ||
| super(1, decorateThreadFactory(threadFactory, name, handler)); |
There was a problem hiding this comment.
I like your last approach better, but I can't think of a good way to replace the inheritance in DefaultRunLoop. We might want to ponder a little more about this, but I don't think we should hold up this PR on it. If anything, we could argue that this derisks this change since it keeps the existing pattern.
| executor = new FirebaseScheduledExecutor(threadFactory, "firebase-database-worker") { | ||
| @Override | ||
| protected void afterExecute(Runnable runnable, Throwable throwable) { | ||
| if (throwable == null && runnable instanceof Future<?>) { |
There was a problem hiding this comment.
Please call super.afterExecute().
* Adding the sync APIs for FirebaseAuth * Added more tests; Added sync APIs for FirebaseMessaging * Removing Task references from database, iid and fcm APIs * Fixing a typo * Minor code clean up * Updated javadocs; Renamed internal helpers of FirebaseMessaging for consistency * Removed the deprecated FirebaseCredential API (#149) * Removing the Task API (#152) * Removed the deprecated FirebaseCredential API * Removing the deprecated Task API * Dropping Support for App Engine Java 7 Runtime (#153) * Dropping support for GAE 7 * Removed GaeThreadFactory, GaeExecutorService and RevivingScheduledExecutor * Removed the deprecated FirebaseCredential API * Removing GAE java7 related APIs (GaeThreadFactory, RevivingScheduledExecutor) * Removed GaePlatform implementation * Added FirebaseScheduledExecutor * Updated documentation * Some minor nits from code reviews * Calling super method in DefaultRunLoop executor * Removing Deprecated LogWrapper API (#154) * Dropping support for GAE 7 * Removed GaeThreadFactory, GaeExecutorService and RevivingScheduledExecutor * Removed the deprecated FirebaseCredential API * Removing GAE java7 related APIs (GaeThreadFactory, RevivingScheduledExecutor) * Removed GaePlatform implementation * Added FirebaseScheduledExecutor * Updated documentation * Removing LogWrapper API * Removing PrefixedLogger * Removing test config file * Updated CHANGELOG * Minor clean ups pointed out in the code review * Minor API doc fixes (#171)
2 participants
In order to support Java 7 runtime in App Engine, we had implemented a number of abstractions and workarounds:
GaeThreadFactoryandGaeExecutorServicefor leveraging background threads supportGaePlatformclass for injecting the GAE threads into RTDB client codeRevivingScheduledExecutorfor periodically restarting threads, to work around GAE thread deadlinesWith the new Java 8 runtime, none of the above is necessary. We can use regular threads instead of the expensive background threads, and the threads are no longer killed abruptly by the runtime (verified via long running tests of this PR).