ctubbsii commented on code in PR #5451:
URL: https://github.com/apache/accumulo/pull/5451#discussion_r2045281834
##########
core/src/main/java/org/apache/accumulo/core/util/tables/TableZooHelper.java:
##########
@@ -62,82 +55,84 @@ public TableZooHelper(ClientContext context) {
* getCause() of NamespaceNotFoundException
*/
public TableId getTableId(String tableName) throws TableNotFoundException {
- for (AccumuloTable systemTable : AccumuloTable.values()) {
- if (systemTable.tableName().equals(tableName)) {
- return systemTable.tableId();
- }
+ Pair<String,String> qualified = TableNameUtil.qualify(tableName);
+ NamespaceId nid =
context.getNamespaces().getNameToIdMap().get(qualified.getFirst());
+ if (nid == null) {
+ throw new TableNotFoundException(tableName, new
NamespaceNotFoundException(null,
+ qualified.getFirst(), "No mapping found for namespace"));
}
- try {
- return
_getTableIdDetectNamespaceNotFound(EXISTING_TABLE_NAME.validate(tableName));
- } catch (NamespaceNotFoundException e) {
- throw new TableNotFoundException(tableName, e);
+ TableId tid =
context.getTableMapping(nid).getNameToIdMap().get(qualified.getSecond());
+ if (tid == null) {
+ throw new TableNotFoundException(null, tableName,
+ "No entry for this table found in the given namespace mapping");
}
+ return tid;
}
- /**
- * Lookup table ID in ZK. If not found, clears cache and tries again.
- */
- public TableId _getTableIdDetectNamespaceNotFound(String tableName)
- throws NamespaceNotFoundException, TableNotFoundException {
- TableId tableId = getTableMap().getNameToIdMap().get(tableName);
- if (tableId == null) {
- // maybe the table exist, but the cache was not updated yet...
- // so try to clear the cache and check again
- clearTableListCache();
- tableId = getTableMap().getNameToIdMap().get(tableName);
- if (tableId == null) {
- String namespace = TableNameUtil.qualify(tableName).getFirst();
- if (Namespaces.getNameToIdMap(context).containsKey(namespace)) {
- throw new TableNotFoundException(null, tableName, null);
- } else {
- throw new NamespaceNotFoundException(null, namespace, null);
- }
+ public String getTableName(TableId tableId) throws TableNotFoundException {
+ Map<NamespaceId,String> namespaceMapping =
context.getNamespaces().getIdToNameMap();
+ for (NamespaceId namespaceId : namespaceMapping.keySet()) {
+ var tableIdToNameMap =
context.getTableMapping(namespaceId).getIdToNameMap();
+ if (tableIdToNameMap.containsKey(tableId)) {
+ return TableNameUtil.qualified(tableIdToNameMap.get(tableId),
+ namespaceMapping.get(namespaceId));
}
}
- return tableId;
+ throw new TableNotFoundException(tableId.canonical(), null,
+ "No entry for this table Id found in table mappings");
}
- public String getTableName(TableId tableId) throws TableNotFoundException {
- for (AccumuloTable systemTable : AccumuloTable.values()) {
- if (systemTable.tableId().equals(tableId)) {
- return systemTable.tableName();
+ private Map<String,String> loadQualifiedTableMapping(boolean reverse) {
Review Comment:
> My understanding is that the old code used to cache the table mapping for
10 mins and that the id -> name and name -> id maps were derived from this
cached table mapping.
I think the old TableMap code is entirely replaced by TableMapping, which
does keep a consistent view of both of them internally. It could return them
together as a snapshot, in case the caller needs both. But, I suspect that most
callers will need one or the other, but not both. The two methods to get the
forward and reverse views could be replaced by a single method that returns a
TableMappingSnapshot (which is basically a
`Pair<Map<TableId,String>,Map<String,TableId>>`).
> It looks like in this version, a user calling
`ClientContext.getTableNameToIdMap()` and `ClientContext.getTableIdToNameMap()`
could get inconsistent information (the map entries are not the same) if a
table was created or deleted between the two ClientContext method calls. Is
that right?
I think most callers would use only one of these, so it's not important that
they both return the same thing at this level. But, one would have to check the
callers of these methods to verify. (Note for myself: these methods provide a
qualified view of the table names, which differs from the TableMapping, which
contains only the unqualified table names without the namespace name)
>
> If that's correct, then it might make sense to have this method populate
both maps with the same information and to store both maps in different
Suppliers with expiration (see Suppliers.memoizeWithExpiration).
I think you mean a single supplier that populates both? I don't think we
have places where callers need both, but we would want to avoid caching the
qualified name in a way that doesn't expire it when the namespace name changes.
Caching at the TableMapping is probably sufficient, and just have the caller
that makes the names qualified with the namespace present it as an on-demand
view, rather than cache again. If we want to keep the caching centralized to
the TableMapping, could add a `getQualifiedSnapshot(String namespaceName)`
method that computes a qualified view using the provided namespace name, and
caches the computation inside TableMapping until a different namespace name is
provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
