bbeaudreault commented on code in PR #4925:
URL: https://github.com/apache/hbase/pull/4925#discussion_r1051517796
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerStateStore.java:
##########
@@ -15,60 +15,52 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.hadoop.hbase.zookeeper;
+package org.apache.hadoop.hbase.master.balancer;
import java.io.IOException;
-import org.apache.hadoop.hbase.Abortable;
import org.apache.hadoop.hbase.exceptions.DeserializationException;
-import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.master.MasterStateStore;
+import org.apache.hadoop.hbase.master.region.MasterRegion;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.zookeeper.KeeperException;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
import org.apache.hadoop.hbase.shaded.protobuf.generated.LoadBalancerProtos;
/**
- * Tracks the load balancer state up in ZK
+ * Store the balancer state.
*/
@InterfaceAudience.Private
-public class LoadBalancerTracker extends ZKNodeTracker {
- private static final Logger LOG =
LoggerFactory.getLogger(LoadBalancerTracker.class);
+public class LoadBalancerStateStore extends MasterStateStore {
Review Comment:
should this class just use SwitchStateStore instead? implementation looks
the same, unless I'm missing a key difference
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterStateStore.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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
+ *
+ * http://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.hadoop.hbase.master;
+
+import static
org.apache.hadoop.hbase.master.region.MasterRegionFactory.STATE_FAMILY;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.client.Get;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.client.Result;
+import org.apache.hadoop.hbase.master.region.MasterRegion;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.zookeeper.ZKUtil;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A state storage which stores the state in master local region.
+ * <p/>
+ * We used to store some persistent state on zookeeper, so here we provide the
ability to migrate
+ * the state from zookeeper.
+ * <p/>
+ * Since parsing the state may introduce some extra overhead, we make this
class abstract and the
+ * get and set state methods protected. Sub classes should store their state
in decoded format to
+ * save the extra parsing overhead.
+ */
[email protected]
+public abstract class MasterStateStore {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(MasterStateStore.class);
+
+ private static final byte[] QUALIFIER = Bytes.toBytes("d");
+
+ private final MasterRegion masterRegion;
+
+ private final byte[] stateName;
+
+ private Result get() throws IOException {
+ return masterRegion.get(new Get(stateName).addColumn(STATE_FAMILY,
QUALIFIER));
+ }
+
+ private void update(byte[] s) throws IOException {
+ masterRegion.update(r -> r.put(new Put(stateName).addColumn(STATE_FAMILY,
QUALIFIER, s)));
+ }
+
+ private byte[] migrate(ZKWatcher watcher, String zkPath) throws
KeeperException, IOException {
+ byte[] zkData = ZKUtil.getDataNoWatch(watcher, zkPath, null);
+ if (zkData == null || zkData.length == 0) {
+ return null;
+ }
+ update(zkData);
+ return zkData;
+ }
+
+ private void tryMigrate(ZKWatcher watcher, String zkPath) throws
IOException, KeeperException {
+ Result result = masterRegion.get(new
Get(stateName).addColumn(STATE_FAMILY, QUALIFIER));
+ if (result.isEmpty()) {
+ // migrate
+ migrate(watcher, zkPath);
+ }
+ // we may fail in the middle so even if the value is available in master
local region, we may
+ // still leave a znode on zookeeper, so always try to delete the znode
here since it is not very
+ // expensive
+ try {
+ ZKUtil.deleteNodeFailSilent(watcher, zkPath);
+ } catch (Exception e) {
+ LOG.warn("failed to delete migrated zk state node {}, ignore and
continue", zkPath);
+ }
+ }
Review Comment:
can you move these private methods below the protected methods so it reads a
little more naturally?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZNodePaths.java:
##########
@@ -69,10 +69,25 @@ public class ZNodePaths {
// znode used for log splitting work assignment
public final String splitLogZNode;
// znode containing the state of the load balancer
+ /**
+ * @deprecated Since 2.6.0, will be removed in 4.0.0. We use master local
region to store this
Review Comment:
this class is IA.Private. Do we need a long deprecation period?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterStateStore.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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
+ *
+ * http://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.hadoop.hbase.master;
+
+import static
org.apache.hadoop.hbase.master.region.MasterRegionFactory.STATE_FAMILY;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.client.Get;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.client.Result;
+import org.apache.hadoop.hbase.master.region.MasterRegion;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.zookeeper.ZKUtil;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A state storage which stores the state in master local region.
+ * <p/>
+ * We used to store some persistent state on zookeeper, so here we provide the
ability to migrate
+ * the state from zookeeper.
+ * <p/>
+ * Since parsing the state may introduce some extra overhead, we make this
class abstract and the
+ * get and set state methods protected. Sub classes should store their state
in decoded format to
+ * save the extra parsing overhead.
+ */
[email protected]
+public abstract class MasterStateStore {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(MasterStateStore.class);
+
+ private static final byte[] QUALIFIER = Bytes.toBytes("d");
+
+ private final MasterRegion masterRegion;
+
+ private final byte[] stateName;
+
+ private Result get() throws IOException {
+ return masterRegion.get(new Get(stateName).addColumn(STATE_FAMILY,
QUALIFIER));
+ }
+
+ private void update(byte[] s) throws IOException {
+ masterRegion.update(r -> r.put(new Put(stateName).addColumn(STATE_FAMILY,
QUALIFIER, s)));
+ }
+
+ private byte[] migrate(ZKWatcher watcher, String zkPath) throws
KeeperException, IOException {
+ byte[] zkData = ZKUtil.getDataNoWatch(watcher, zkPath, null);
+ if (zkData == null || zkData.length == 0) {
+ return null;
+ }
+ update(zkData);
+ return zkData;
+ }
+
+ private void tryMigrate(ZKWatcher watcher, String zkPath) throws
IOException, KeeperException {
+ Result result = masterRegion.get(new
Get(stateName).addColumn(STATE_FAMILY, QUALIFIER));
Review Comment:
re-use `get()` method from above?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/RegionNormalizerStateStore.java:
##########
@@ -15,60 +15,52 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.hadoop.hbase.zookeeper;
+package org.apache.hadoop.hbase.master.normalizer;
import java.io.IOException;
-import org.apache.hadoop.hbase.Abortable;
import org.apache.hadoop.hbase.exceptions.DeserializationException;
-import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.master.MasterStateStore;
+import org.apache.hadoop.hbase.master.region.MasterRegion;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.zookeeper.KeeperException;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
import
org.apache.hadoop.hbase.shaded.protobuf.generated.RegionNormalizerProtos;
/**
- * Tracks region normalizer state up in ZK
+ * Store region normalizer state.
*/
@InterfaceAudience.Private
-public class RegionNormalizerTracker extends ZKNodeTracker {
- private static final Logger LOG =
LoggerFactory.getLogger(RegionNormalizerTracker.class);
+public class RegionNormalizerStateStore extends MasterStateStore {
Review Comment:
this also looks like it should use SwitchStateStore? would be nice to unify
all these switches. just needs an abstract toByteArray/parseFrom
--
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]
