dlmarion commented on code in PR #5451:
URL: https://github.com/apache/accumulo/pull/5451#discussion_r2042840592
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/PopulateZookeeper.java:
##########
@@ -53,24 +53,23 @@ public Repo<Manager> call(FateId fateId, Manager manager)
throws Exception {
Utils.getTableNameLock().lock();
try {
- // write tableName & tableId to zookeeper
- Utils.checkTableNameDoesNotExist(manager.getContext(),
tableInfo.getTableName(),
- tableInfo.getNamespaceId(), tableInfo.getTableId(),
TableOperation.CREATE);
-
+ var context = manager.getContext();
+ // write tableName & tableId, first to Table Mapping and then to
Zookeeper
+ context.getTableMapping(tableInfo.getNamespaceId()).put(context,
tableInfo.getTableId(),
Review Comment:
If an exception is thrown in the code below, do we need to remove the table
frmo the table mapping?
##########
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.
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?
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).
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader12to13.java:
##########
@@ -399,4 +403,37 @@ private void
validateEmptyZKWorkerServerPaths(ServerContext context) {
}
}
}
+
+ void addTableMappingsToZooKeeper(ServerContext context) {
+ var zrw = context.getZooSession().asReaderWriter();
+ try {
+ List<String> tableIds = zrw.getChildren(Constants.ZTABLES);
+ Map<String,Map<String,String>> mapOfTableMaps = new HashMap<>();
+
+ for (String tableId : tableIds) {
Review Comment:
We need to remove `ZTABLE_NAME` too, right?
##########
server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java:
##########
@@ -178,7 +172,16 @@ public void cloneTable(TableId srcTableId, TableId
tableId, String tableName,
PropUtil.removeProperties(context, TablePropKey.of(tableId),
propertiesToExclude);
}
- public void removeTable(TableId tableId) throws KeeperException,
InterruptedException {
+ public void removeTable(TableId tableId, NamespaceId namespaceId)
+ throws KeeperException, InterruptedException,
AcceptableThriftTableOperationException {
+ try {
Review Comment:
`removeTable` removes the table mapping, but the other methods update the
table mapping in the fate steps. Personally, I like the table mapping updates
here, but I think we should be consistent with it.
##########
core/src/main/java/org/apache/accumulo/core/metadata/AccumuloTable.java:
##########
@@ -36,25 +36,39 @@ public enum AccumuloTable {
SCAN_REF("scanref", "+scanref");
private final String name;
+ private final String simpleName;
private final TableId tableId;
public String tableName() {
return name;
}
+ public String simpleTableName() {
+ return simpleName;
+ }
+
public TableId tableId() {
return tableId;
}
AccumuloTable(String name, String id) {
this.name = Namespace.ACCUMULO.name() + "." + name;
+ this.simpleName = name;
this.tableId = TableId.of(id);
}
private static final Set<TableId> ALL_IDS =
Arrays.stream(values()).map(AccumuloTable::tableId).collect(Collectors.toUnmodifiableSet());
+ private static final Set<TableId> BUILT_IN_IDS =
Review Comment:
How is this different than `ALL_IDS` ?
--
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
