Add ZooKeeper Namespace Id-to-name mapping #4996
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In progress of fixing Upgrader11to12Test which failed the unit test checks. |
|
Upgrader11to12Test has been fixed and passes now. |
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java
Outdated
Show resolved
Hide resolved
...ger/src/main/java/org/apache/accumulo/manager/tableOps/namespace/rename/RenameNamespace.java
Outdated
Show resolved
Hide resolved
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java
Show resolved
Hide resolved
ctubbsii
requested changes
Oct 25, 2024
server/base/src/main/java/org/apache/accumulo/server/init/ZooKeeperInitializer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/init/ZooKeeperInitializer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java
Outdated
Show resolved
Hide resolved
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java
Outdated
Show resolved
Hide resolved
...ger/src/main/java/org/apache/accumulo/manager/tableOps/namespace/rename/RenameNamespace.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
TableManager.removeNameSpace() needs to be updated to remove the namespace id from the new json. Also that function seems like its in the wrong place.
The following comment is not a suggested for this PR its a possible follow on, this was a preexisting problem in the code. The code seems to construct the paths for namespaces in ZK all over the place. It would be nice to corral this path construction into more centralized code like maybe in Namesspace or NamespaceMapping code. The recent service lock path code corraled zk path construction and centralized, makes the code easier to reason about and collocates path construction code w/ ZK update/read code.
core/src/main/java/org/apache/accumulo/core/clientImpl/Namespaces.java
Outdated
Show resolved
Hide resolved
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java
Outdated
Show resolved
Hide resolved
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java
Show resolved
Hide resolved
- Implement Json-formatted mapping of namespace Ids to Names in ZooKeeper. Stored on the "/namespaces" node. Initially created in ZooKeeperInitializer. - Create NamespaceMapping.java to hold methods initializing, adding, retrieving, serializing, and deserializing namespace Id to Name map in ZooKeeper. - Modify all reading and writing of the "/name" nodes so that code instead reads and writes to the new Json mapping for namespace name data. - Deprecate ZNAMESPACE_NAME constant, used for the "/name" node. - Build namespace id to name mapping of current ZooKeeper namespaces in Upgrader11to12. Delete all current "/name" nodes. - Add test code to Upgrader11to12Test respective to additions made to Upgrader11to12. Resolves: apache#4698
- Move SuppressWarnings annotation for handling deprecated Constants.ZNAMESPACE_NAME Resolves: apache#4698
- Add functionality to serialize() in NamespaceMapping that sorts the serialized mapping. Should not significantly impact the issue overall - but resolves the test that was failing in Upgrader11to12Test due to AssertionError caused by comparing 2 unsorted maps.
- Move deletion of name nodes to after the namespace mapping has been computed and persisted in ZooKeeper. - Check for existing namespace mapping in upgrade code. If does not exist, compute mapping and delete name nodes. If already exists, do not recompute namespace mapping but still delete name nodes. - Check for empty or null namespace mapping when deserializing. If empty or null, explicitly return empty Map<String,String>. Resolves: apache#4698
- Move Constants.ZNAMESPACE_NAME to Upgrader11to12 instead of deprecating. - Throw AssertionError when deserializing map if data is null. - Remove call to Zoocache.clear(). - Modify Namespaces.exists() to look up namespace Ids in new mapping instead of looking them up in ZK. - Use mutateExisting() in NamespaceMapping.writeNamespaceToMap(). Include TODO for checking if namespace name already exists. - Fix usage of new mapping in use of mutateExisting in RenameNamespace. - Revert NodeExistsPolicy.OVERWRITE to FAIL when creating nodes in ZooKeeperInitializer.initialize. - Move intializeNamespaceMap directly to creation of namespaces node. Resolves: apache#4698
ctubbsii
force-pushed
the
zk-ns-mapping
branch
from
727dc36
to
6c39dd6
Compare
- Check if /namespaces node holds unexpected data in Upgrader11to12 code. If found, throw exception. - Streamline Namespaces.exists(). - Add check for accumulo/default namespace when retrieving updated mapping. Throw exception if not present. - Refactor the following actions on namespaces: create, rename, remove. Move all functionality that updates the new mapping to be in NamespaceMapping, use mutateExisting(), and add checks for namespaces that may already exist/do not exist, depending on the action. - Remove now-unneeded Utils.checkNamespaceDoesNotExist() and NamespaceMapping.initializeNamespaceMap(). Resolves: apache#4698
- Inline path String in RenameNamespace, store context.getManager() in variable in PopulateZooKeeperWithNamespace Resolves: apache#4698
|
Commit dd8c4fa has moved all namespace mapping actions (create/put, rename, remove) into NamespaceMapping, ensured that mutateExisting() is used in all cases, and gives checks for non-existent/already-existing namespaces (depending on the action). The actual namespace node creation and removal functionalities remain in their respective classes and occur after the mapping is updated. |
keith-turner
reviewed
Nov 5, 2024
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java
Show resolved
Hide resolved
* Use TreeMap for type token for both serialization and deserialization * Use global Gson from LazySingletons * Add checks for built-in namespaces for rename and remove cases * Move name already existing check for rename into the mutateExisting method * Add unit test * Remove unused exception from throws * Inline prepareNewNamespaceState overloaded method * Revert adding some extra params to TableManager.removeNamespace * Prevent namespace not found from blocking removal operation of namespace
ctubbsii
approved these changes
Nov 14, 2024
asfgit
pushed a commit
that referenced
this pull request
Nov 15, 2024
This merges #4996 into the main branch
|
@meatballspaghetti - please take a look at NamespacesIT, I'm seeing 4 tests fail with the same type of error: |
ctubbsii
added a commit
to ctubbsii/accumulo
that referenced
this pull request
Nov 15, 2024
* Add a check to make sure that if the name exists in the mapping, then it results in a namespace exists exception, rather than only do that if the ID exists in the mapping (this fixes the NamespaceIT failure seen after apache#4996 was merged) * Make NamespaceMapping.put() idempotent by checking the current mapping against the desired mapping (this preserves existing behavior at this point in the code, which was previously being handled by the Utils method that was deleted) Trivial changes: * Add some stricter checks for parameters to NamespaceMapping methods (final and require non-null) * Avoid NPE problems by comparing the known non-null string on the left side of the .equals() * Make name and inline comments consistent between rename and put methods
I didn't see that specific stack trace when running NamespacesIT in the 3.1 branch. I got different exceptions instead. Perhaps that stack trace only appears in the main branch? I did fix the test failures I saw in 3.1 with #5067. I tested a local merge to the main branch and NamespacesIT passed there also. |
ctubbsii
added a commit
that referenced
this pull request
Nov 15, 2024
* Add a check to make sure that if the name exists in the mapping, then it results in a namespace exists exception, rather than only do that if the ID exists in the mapping (this fixes the NamespaceIT failure seen after #4996 was merged) * Make NamespaceMapping.put() idempotent by checking the current mapping against the desired mapping (this preserves existing behavior at this point in the code, which was previously being handled by the Utils method that was deleted) Trivial changes: * Add some stricter checks for parameters to NamespaceMapping methods (final and require non-null) * Avoid NPE problems by comparing the known non-null string on the left side of the .equals() * Make name and inline comments consistent between rename and put methods
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is part of an ongoing work to improve efficiency and reduce slowdown of retrieving and writing namespace names and table names in ZooKeeper. These particular changes modify the namespace names, and a follow-up PR in the near future will modify the table names. It replaces the use of the
/namenodes under namespaces with a namespace id-to-name mapping in a Json format stored under the/namespacesnode. With this, when any function wants to retrieve a namespace name, ZooKeeper will only need to read the single mapping instead of iterating through all/namenodes every time.New files added for implementation:
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceMapping.java: Class that holds functionality for initializing, retrieving, writing, serializing, and deserializing namespace id-to-name map (and name-to-id map).Files changed for implementation:
ClientContext.java: Allow namespace mapping to be retrieved from ZooKeeper by referencing the context.Namespaces.java: Update namespace name retrieval functionality with new mapping.ZooKeeperInitializer.java: Replace initialization ofdefaultandaccumulo/namenodes with creation of new mapping.TableManager.java: Update namespace creation functionality to use new mapping.RenameNamespace.java: Replace usage of/namenode to instead use new mapping.Files changed for upgrading:
Constants.java: DeprecateZNAMESPACE_NAME, as the/namenode will no longer be used.Upgrader11to12.java: Build namespace id-to-name mapping in ZooKeeper out of current/namenodes. Follow with deletion of/namenodes.Upgrader11to12Test.java: Add test code respective to additions made inUpgrader11to12.java.Original Issue: #4698