Repository navigation
Conversation
![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 refactors the Bigtable client shims to share a unified SessionPoolMap registry, partitioning session-pool handles by target ID and permission using a new SessionKey class. This allows different shims to share the same session pool for the same target and permission. However, the supports() methods in CheckAndMutateRowShim, MutateRowShim, ReadModifyWriteRowShim, and ReadRowShim currently call sessionMap.get(), which eagerly loads and opens the session pools during support checks. It is recommended to use sessionMap.getIfPresent() instead to preserve the lazy-loading design and avoid unnecessary connection overhead.
| SessionPool<?> pool; | ||
| // TODO: avoid double lookup | ||
| if (targetId instanceof TableId) { | ||
| pool = tables.get((TableId) targetId).getSessionPool(); | ||
| pool = sessionMap.get((TableId) targetId, SessionKey.Permission.WRITE).getSessionPool(); | ||
| } else if (targetId instanceof AuthorizedViewId) { | ||
| pool = authViews.get((AuthorizedViewId) targetId).getSessionPool(); | ||
| pool = | ||
| sessionMap.get((AuthorizedViewId) targetId, SessionKey.Permission.WRITE).getSessionPool(); | ||
| } else { |
There was a problem hiding this comment.
The supports() method should use sessionMap.getIfPresent() instead of sessionMap.get() to avoid eagerly loading/opening the session pool during support checks. Eagerly loading the pool defeats the lazy-loading design and can cause unnecessary connection overhead or unexpected exceptions during inspection.
SessionPool<?> pool;
if (targetId instanceof TableId) {
com.google.cloud.bigtable.data.v2.internal.api.TableAsync table =
sessionMap.getIfPresent((TableId) targetId, SessionKey.Permission.WRITE);
if (table == null) {
return true;
}
pool = table.getSessionPool();
} else if (targetId instanceof AuthorizedViewId) {
com.google.cloud.bigtable.data.v2.internal.api.AuthorizedViewAsync view =
sessionMap.getIfPresent((AuthorizedViewId) targetId, SessionKey.Permission.WRITE);
if (view == null) {
return true;
}
pool = view.getSessionPool();
} else {| SessionPool<?> pool; | ||
| if (targetId instanceof TableId) { | ||
| pool = tables.get((TableId) targetId).getSessionPool(); | ||
| pool = sessionMap.get((TableId) targetId, SessionKey.Permission.READ_WRITE).getSessionPool(); | ||
| } else if (targetId instanceof AuthorizedViewId) { | ||
| pool = authViews.get((AuthorizedViewId) targetId).getSessionPool(); | ||
| pool = | ||
| sessionMap | ||
| .get((AuthorizedViewId) targetId, SessionKey.Permission.READ_WRITE) | ||
| .getSessionPool(); | ||
| } else { |
There was a problem hiding this comment.
The supports() method should use sessionMap.getIfPresent() instead of sessionMap.get() to avoid eagerly loading/opening the session pool during support checks. Eagerly loading the pool defeats the lazy-loading design and can cause unnecessary connection overhead or unexpected exceptions during inspection.
SessionPool<?> pool;
if (targetId instanceof TableId) {
com.google.cloud.bigtable.data.v2.internal.api.TableAsync table =
sessionMap.getIfPresent((TableId) targetId, SessionKey.Permission.READ_WRITE);
if (table == null) {
return true;
}
pool = table.getSessionPool();
} else if (targetId instanceof AuthorizedViewId) {
com.google.cloud.bigtable.data.v2.internal.api.AuthorizedViewAsync view =
sessionMap.getIfPresent((AuthorizedViewId) targetId, SessionKey.Permission.READ_WRITE);
if (view == null) {
return true;
}
pool = view.getSessionPool();
} else {| SessionPool<?> pool; | ||
| // TODO avoid double lookup | ||
| if (targetId instanceof TableId) { | ||
| pool = tables.get((TableId) targetId).getSessionPool(); | ||
| pool = sessionMap.get((TableId) targetId, SessionKey.Permission.READ).getSessionPool(); | ||
| } else if (targetId instanceof AuthorizedViewId) { | ||
| pool = authViews.get((AuthorizedViewId) targetId).getSessionPool(); | ||
| pool = | ||
| sessionMap.get((AuthorizedViewId) targetId, SessionKey.Permission.READ).getSessionPool(); | ||
| } else if (targetId instanceof MaterializedViewId) { | ||
| pool = matViews.get((MaterializedViewId) targetId).getSessionPool(); | ||
| pool = | ||
| sessionMap | ||
| .get((MaterializedViewId) targetId, SessionKey.Permission.READ) | ||
| .getSessionPool(); | ||
| } else { |
There was a problem hiding this comment.
The supports() method should use sessionMap.getIfPresent() instead of sessionMap.get() to avoid eagerly loading/opening the session pool during support checks. Eagerly loading the pool defeats the lazy-loading design and can cause unnecessary connection overhead or unexpected exceptions during inspection.
SessionPool<?> pool;
if (targetId instanceof TableId) {
com.google.cloud.bigtable.data.v2.internal.api.TableAsync table =
sessionMap.getIfPresent((TableId) targetId, SessionKey.Permission.READ);
if (table == null) {
return true;
}
pool = table.getSessionPool();
} else if (targetId instanceof AuthorizedViewId) {
com.google.cloud.bigtable.data.v2.internal.api.AuthorizedViewAsync view =
sessionMap.getIfPresent((AuthorizedViewId) targetId, SessionKey.Permission.READ);
if (view == null) {
return true;
}
pool = view.getSessionPool();
} else if (targetId instanceof MaterializedViewId) {
com.google.cloud.bigtable.data.v2.internal.api.MaterializedViewAsync view =
sessionMap.getIfPresent((MaterializedViewId) targetId, SessionKey.Permission.READ);
if (view == null) {
return true;
}
pool = view.getSessionPool();
} else {
fc9b8d8 to
92b19d1
Compare
…sionMap into SessionPoolMap
Add SessionPermission (READ/WRITE/READ_WRITE) and SessionKey<IdT> to
pair a target ID with a permission level, then absorb the three-map
registry logic directly into SessionPoolMap instead of a separate class.
SessionPoolMap now holds one LoadingCache per ID type
(TableId / AuthorizedViewId / MaterializedViewId), keyed by
SessionKey<IdT>. Two shim operations requesting the same (id,
permission) — e.g. a future CheckAndMutateRowShim and
ReadModifyWriteRowShim both using READ_WRITE on the same table — will
resolve to the same handle and therefore the same session pool.
Fix supports() to use getIfPresent() instead of get(). Previously
supports() called the loading get(), which would:
1. Open a session pool as a side effect of a cheap read-only check.
2. Throw IllegalStateException (Client is closed) unchecked through
DivertingUnaryCallable.useExperimental(), bypassing the normal
failed-future error surface.
With getIfPresent(), a null return (no pool yet) is treated as
"optimistically supported": the pool opens lazily in call(), and if
that fails (closed client, etc.) apply() surfaces it as a failed future.
ShimImpl creates a single shared SessionPoolMap and passes it to all
shims; each shim declares its required SessionPermission internally.
This branch has not been deployed
1 participant
No description provided.