wchevreuil commented on code in PR #8380: URL: https://github.com/apache/hbase/pull/8380#discussion_r3498098671
########## hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/ReplicationBulkLoadEventCleaner.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.cleaner; + +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.ScheduledChore; +import org.apache.hadoop.hbase.Stoppable; +import org.apache.hadoop.hbase.replication.regionserver.ReplicationBulkLoadEventTracker; +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; + +/** + * Cleans completed replicated bulk load event markers after they are old enough. + */ [email protected] +public class ReplicationBulkLoadEventCleaner extends ScheduledChore { + + private static final Logger LOG = LoggerFactory.getLogger(ReplicationBulkLoadEventCleaner.class); + + public static final String PERIOD_MS_KEY = + "hbase.master.cleaner.replication.bulkload.event.period.ms"; + public static final int PERIOD_MS_DEFAULT = (int) TimeUnit.MINUTES.toMillis(10); + public static final String DONE_TTL_MS_KEY = "hbase.replication.bulkload.event.done.ttl.ms"; + public static final long DONE_TTL_MS_DEFAULT = TimeUnit.DAYS.toMillis(1); + + private final ReplicationBulkLoadEventTracker tracker; + private final long doneTtlMs; + + public ReplicationBulkLoadEventCleaner(Configuration conf, Stoppable stopper, ZKWatcher zkw) { + super("ReplicationBulkLoadEventCleaner", stopper, + conf.getInt(PERIOD_MS_KEY, PERIOD_MS_DEFAULT)); + this.tracker = new ReplicationBulkLoadEventTracker(conf, zkw); + this.doneTtlMs = conf.getLong(DONE_TTL_MS_KEY, DONE_TTL_MS_DEFAULT); + } + + @Override + protected void chore() { + try { + int deleted = tracker.cleanDoneMarkers(doneTtlMs); + if (deleted > 0) { + LOG.info("Cleaned {} replicated bulkload event marker(s)", deleted); + } + } catch (KeeperException e) { + LOG.warn("Failed to clean replicated bulkload event markers", e); + } + } + + public void choreForTesting() { Review Comment: There's no need for this extra method. Tests in the same package can simply call chore() directly. And the super class already defines same method, only difference being it's synchronized there. ########## hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationBulkLoadEventTracker.java: ########## @@ -0,0 +1,268 @@ +/* + * 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.replication.regionserver; + +import java.io.IOException; +import java.io.InterruptedIOException; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Objects; +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; +import org.apache.hadoop.hbase.util.MD5Hash; +import org.apache.hadoop.hbase.zookeeper.ZKUtil; +import org.apache.hadoop.hbase.zookeeper.ZKWatcher; +import org.apache.hadoop.hbase.zookeeper.ZNodePaths; +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.Stat; + +/** + * Coordinates replicated bulk load events across all region servers in the sink cluster. + */ [email protected] +public class ReplicationBulkLoadEventTracker { Review Comment: Having a tracker for this kind of events is a good idea, but we should not make it tight to ZK. There's been an effort to gradually move HBase features out of ZK. Replication queue storage, for example, already has a ZKless alternative (see HBASE-27109). This tracker, should follow similar approach. We should have it as interface/abstract class and provide both ZK based and table based implementations. However, I don't think this should be implemented in this same PR. My suggestion for you then is to move this tracking logic out of this PR, let's leave this one simply handling the issue on individual RS level. Then once this is merged, you can file a new jira/PR to implement the tracking logic at a global level, covering both ZK and table based registries. ########## hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/ReplicationBulkLoadEventCleaner.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.cleaner; + +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.ScheduledChore; +import org.apache.hadoop.hbase.Stoppable; +import org.apache.hadoop.hbase.replication.regionserver.ReplicationBulkLoadEventTracker; +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; + +/** + * Cleans completed replicated bulk load event markers after they are old enough. + */ Review Comment: Please add explanation about how the new configuration properties influence this chore behaviour. -- 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]
