Repository navigation
fix(bookshelf-standard): don't crash startup on index lag - #1266
saiflayouni wants to merge 1 commit into
Conversation
DatastoreSessionFilter.init() queries for sessions stale by more than
two days and deletes them, as a best-effort cleanup task. On a fresh
deployment, Datastore has no index yet for a property ("lastModified")
it has never seen written, so this query throws
DatastoreNeedIndexException until the first session is actually
written - which took down the whole filter, and therefore the whole
app, at startup (GoogleCloudPlatform#448).
Catch DatastoreNeedIndexException and log a warning instead: there's
nothing to clean up on a Datastore that has never written a session
anyway, so skipping the cleanup in that case is harmless, and letting
an optional maintenance task fail shouldn't prevent the app from
starting.
Same fix applied identically across all four bookshelf-standard steps
(2-structured-data, 3-binary-data, 4-auth, 5-logging), which share
this file verbatim.
Fixes GoogleCloudPlatform#448
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the DatastoreSessionFilter across multiple modules to catch DatastoreNeedIndexException during startup stale-session cleanup, preventing application startup failures on fresh deployments where the Datastore index is not yet ready. The review feedback correctly points out that deleting entities one-by-one in a loop is highly inefficient and can cause startup delays; it recommends batching the delete operations into a single call instead.
| Iterator<Entity> results = datastore.prepare(query).asIterator(); | ||
| while (results.hasNext()) { | ||
| Entity stateEntity = results.next(); | ||
| datastore.delete(stateEntity.getKey()); | ||
| } |
There was a problem hiding this comment.
Performing Datastore delete operations one-by-one in a loop is highly inefficient as it triggers a separate RPC for each entity. Since this cleanup runs synchronously during filter initialization (init()), a large number of stale sessions could cause significant startup delays or timeouts. Batching the deletes into a single datastore.delete(keysToDelete) call is much more efficient and safer.
Iterable<Entity> results = datastore.prepare(query).asIterable();
java.util.List<com.google.appengine.api.datastore.Key> keysToDelete = new java.util.ArrayList<>();
for (Entity stateEntity : results) {
keysToDelete.add(stateEntity.getKey());
}
if (!keysToDelete.isEmpty()) {
datastore.delete(keysToDelete);
}| Iterator<Entity> results = datastore.prepare(query).asIterator(); | ||
| while (results.hasNext()) { | ||
| Entity stateEntity = results.next(); | ||
| datastore.delete(stateEntity.getKey()); | ||
| } |
There was a problem hiding this comment.
Performing Datastore delete operations one-by-one in a loop is highly inefficient as it triggers a separate RPC for each entity. Since this cleanup runs synchronously during filter initialization (init()), a large number of stale sessions could cause significant startup delays or timeouts. Batching the deletes into a single datastore.delete(keysToDelete) call is much more efficient and safer.
Iterable<Entity> results = datastore.prepare(query).asIterable();
java.util.List<com.google.appengine.api.datastore.Key> keysToDelete = new java.util.ArrayList<>();
for (Entity stateEntity : results) {
keysToDelete.add(stateEntity.getKey());
}
if (!keysToDelete.isEmpty()) {
datastore.delete(keysToDelete);
}| Iterator<Entity> results = datastore.prepare(query).asIterator(); | ||
| while (results.hasNext()) { | ||
| Entity stateEntity = results.next(); | ||
| datastore.delete(stateEntity.getKey()); | ||
| } |
There was a problem hiding this comment.
Performing Datastore delete operations one-by-one in a loop is highly inefficient as it triggers a separate RPC for each entity. Since this cleanup runs synchronously during filter initialization (init()), a large number of stale sessions could cause significant startup delays or timeouts. Batching the deletes into a single datastore.delete(keysToDelete) call is much more efficient and safer.
Iterable<Entity> results = datastore.prepare(query).asIterable();
java.util.List<com.google.appengine.api.datastore.Key> keysToDelete = new java.util.ArrayList<>();
for (Entity stateEntity : results) {
keysToDelete.add(stateEntity.getKey());
}
if (!keysToDelete.isEmpty()) {
datastore.delete(keysToDelete);
}| Iterator<Entity> results = datastore.prepare(query).asIterator(); | ||
| while (results.hasNext()) { | ||
| Entity stateEntity = results.next(); | ||
| datastore.delete(stateEntity.getKey()); | ||
| } |
There was a problem hiding this comment.
Performing Datastore delete operations one-by-one in a loop is highly inefficient as it triggers a separate RPC for each entity. Since this cleanup runs synchronously during filter initialization (init()), a large number of stale sessions could cause significant startup delays or timeouts. Batching the deletes into a single datastore.delete(keysToDelete) call is much more efficient and safer.
Iterable<Entity> results = datastore.prepare(query).asIterable();
java.util.List<com.google.appengine.api.datastore.Key> keysToDelete = new java.util.ArrayList<>();
for (Entity stateEntity : results) {
keysToDelete.add(stateEntity.getKey());
}
if (!keysToDelete.isEmpty()) {
datastore.delete(keysToDelete);
}
Summary
DatastoreSessionFilter.init()(in everybookshelf-standardstep) queries for sessions stale by more than two days and deletes them, as a best-effort cleanup task run once when the filter is initialized.On a fresh deployment, Datastore has no index yet for a property it has never seen written — here,
lastModifiedon aSessionVariableentity. Until the very first session is actually written, that query throwsDatastoreNeedIndexException, which propagated straight out ofinit()and took down the whole filter (and therefore the whole app) at startup. This matches #448's report and stack trace exactly (DatastoreSessionFilter.initfailing with "no matching index found" on deploy/local run).Fix
Catch
DatastoreNeedIndexExceptionaround the cleanup query and log a warning instead of propagating. There's nothing to clean up on a Datastore that has never written aSessionVariableanyway, so skipping the cleanup in that case is harmless — and an optional maintenance task failing should never prevent the whole app from starting.Applied identically to all four steps that share this file verbatim:
2-structured-data,3-binary-data,4-auth,5-logging.Test plan
javacagainst each module's full Maven classpath (appengine-api-1.0-sdk, Guava, servlet API, joda-time) — all 4 compile clean.DatastoreNeedIndexException(the exact type the legacycom.google.appengine.api.datastoreSDK throws for "no matching index found") is aRuntimeException, consistent with it propagating unchecked out ofFilter.init()before this fix.Note on the other open issues referenced in this pass
While investigating, I also looked at #393, #419, #1236, and #775 (all currently open). None of them are fixable with a code change in this repository:
Only #448 referenced real, currently-existing, reproducible code, so this PR addresses that one.
Fixes #448