ctubbsii commented on code in PR #3118: URL: https://github.com/apache/accumulo/pull/3118#discussion_r1080655073
########## core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooAclUtil.java: ########## @@ -0,0 +1,166 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.core.fate.zookeeper; Review Comment: The location of ZooUtil is incidental, because it was originally in a separate fate jar. It was not done with conscious planning. A lot of ZooKeeper utils are disorganized, and that's something I've been trying to nudge us towards improvements over the last few versions. We actually used to have two ZooUtil classes, in separate packages, and I was able to get rid of one in 2.1. So, we can either leave it here because that's where other utility code for ZK is located (not my preference)... or we can try to create a better precedent by not coupling it to FaTE when it's not related to FaTE (my preference) so we can be better organized with our utility code. The server-base module has a `zookeeper` package for ZK utilities. Given this is intended to support a server-side CLI utility, that seems to make the most sense to me. `org.apache.accumulo.core.util` is also another place, though it's mostly a big dumping ground for anything that doesn't fit anywhere else, same with the server-ba se module's util package. I don't feel strongly about this, but I think code organization is something we should at least put thought into as we maintain the project. -- 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]
