ctubbsii commented on code in PR #4996:
URL: https://github.com/apache/accumulo/pull/4996#discussion_r1829807739
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java:
##########
@@ -117,6 +122,27 @@ public void upgradeZookeeper(@NonNull ServerContext
context) {
zrw.overwritePersistentData(rootBase, rtm.toJson().getBytes(UTF_8),
stat.getVersion());
log.info("Root metadata in ZooKeeper after upgrade: {}", rtm.toJson());
}
+
+ String zPath = Constants.ZROOT + "/" + context.getInstanceID() +
Constants.ZNAMESPACES;
+ byte[] namespacesData = zrw.getData(zPath);
+ if (namespacesData.length != 0) {
+ throw new IllegalStateException(
+ "Unexpected data found under namespaces node: " + new
String(namespacesData, UTF_8));
+ }
+ List<String> namespaceIdList = zrw.getChildren(zPath);
+ Map<String,String> namespaceMap = new HashMap<>();
+ for (String namespaceId : namespaceIdList) {
+ String namespaceNamePath = zPath + "/" + namespaceId + ZNAMESPACE_NAME;
+ namespaceMap.put(namespaceId, new
String(zrw.getData(namespaceNamePath), UTF_8));
+ }
+ byte[] mapping = NamespaceMapping.serialize(namespaceMap);
+ zrw.putPersistentData(zPath, mapping,
ZooUtil.NodeExistsPolicy.OVERWRITE);
Review Comment:
I'm not sure most of the upgrade code is written to be idempotent. If you
actually encounter data here, it is likely to be very unexpected. Yes, you
could validate it and make the assumption that what's there was created by a
previous step. However, just because it validates doesn't mean that it is a
complete mapping. Consider the case where you attempt an upgrade, it fails, you
resume with the current version and add a few more namespaces (or rename or
delete them)... and then you attempt an upgrade again. Now, it's no longer safe
to assume that just because the mapping validates that it is correct.
So, I think it's much simpler to just throw an exception if there's any kind
of data here, because that's unexpected, and if an exception is thrown, the
corrective action really depends on the content of the data, whether it's
up-to-date, what the user was trying to do at the time, etc. In other words,
it's going to involve an investigation, and possibly surgery.
I think the best course of action is to just throw the exception here. It is
very unlikely to be seen, and attempts to try to mitigate it are only going to
be making assumptions that could very easily be bad assumptions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
