ctubbsii commented on code in PR #5349:
URL: https://github.com/apache/accumulo/pull/5349#discussion_r1966159683
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java:
##########
@@ -134,22 +134,20 @@ public void upgradeZookeeper(@NonNull ServerContext
context) {
String zPath = context.getZooKeeperRoot() + 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);
+ if (namespacesData.length == 0) {
+ 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);
- for (String namespaceId : namespaceIdList) {
- String namespaceNamePath = zPath + "/" + namespaceId + ZNAMESPACE_NAME;
- zrw.delete(namespaceNamePath);
+ for (String namespaceId : namespaceIdList) {
+ String namespaceNamePath = zPath + "/" + namespaceId +
ZNAMESPACE_NAME;
+ zrw.delete(namespaceNamePath);
+ }
Review Comment:
This is nearly identical to the code suggested by @keith-turner in #4996. I
replied with https://github.com/apache/accumulo/pull/4996#discussion_r1829807739
In short, it is not safe to assume that a non-zero length of data here is a
complete and valid mapping.
If this fails, it could have failed for any number of unpredictable reasons.
The current code behavior will catch it and force the user to deal with the
unique and special circumstances that apply to them. This change, however,
makes a lot of assumptions about the current contents being the result of a
very specific situation that you encountered due to another bug in the upgrade
process. That situation has already been fixed. If this is encountered again,
it is likely a completely different scenario, and you can't make assumptions
about that new scenario and why it failed.
It's nice to try to make the upgrade code more idempotent, but it's very
hard to do that correctly and safely. We really need to make sure the upgrade
works without failing. But, if it does fail, it's more important to detect it,
and troubleshoot why. We really shouldn't be making guesses here.
--
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]
