krishan1390 commented on code in PR #18380: URL: https://github.com/apache/pinot/pull/18380#discussion_r3166041689
########## pinot-common/src/main/java/org/apache/pinot/common/utils/helix/ZkMultiWriteBuilder.java: ########## @@ -0,0 +1,147 @@ +/** + * 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.pinot.common.utils.helix; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.List; +import org.apache.helix.zookeeper.api.client.RealmAwareZkClient; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer; +import org.apache.helix.zookeeper.zkclient.exception.ZkException; +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.Op; +import org.apache.zookeeper.ZooDefs; + + +/** + * Fluent builder for an atomic ZooKeeper {@code multi()} transaction. Accumulates ops via + * {@link #set}/{@link #create}/{@link #delete}/{@link #check} and submits them as a single + * all-or-nothing batch on {@link #execute()}. + * + * <p>On atomic rollback (e.g. version mismatch, node missing, node already exists), {@link #execute()} + * throws the underlying {@link KeeperException} subtype ({@code BadVersionException}, + * {@code NoNodeException}, {@code NodeExistsException}, ...). Callers branch on the subtype to + * distinguish retryable concurrent-state changes from hard errors. Per-op offender info is reachable + * via {@link KeeperException#getResults()}. + * + * <p>Connectivity / session failures (timeout, interrupt, session expiry) are not atomic outcomes + * and propagate as the original {@link ZkException}. + * + * <p>Single-use: each instance can be executed at most once. Obtain a fresh builder per transaction + * (typically via {@code PinotHelixResourceManager.multiWriteZK()}). + */ +public final class ZkMultiWriteBuilder { + + /** Pass as {@code expectedVersion} to skip the version check on a {@link #set}/{@link #delete}. */ + public static final int ANY_VERSION = -1; + + // ZNRecordSerializer is documented thread-safe by Helix (no mutable state beyond per-call buffers), + // so a single static instance is fine across concurrent builders. + private static final ZNRecordSerializer SERIALIZER = new ZNRecordSerializer(); + + private final RealmAwareZkClient _zkClient; Review Comment: Rather than RealmAwareZkClient, should it actually just be ZkClient, because that is the abstraction that most Pinot code uses? ########## pinot-common/src/main/java/org/apache/pinot/common/utils/helix/ZkMultiWriteBuilder.java: ########## @@ -0,0 +1,147 @@ +/** + * 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.pinot.common.utils.helix; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.List; +import org.apache.helix.zookeeper.api.client.RealmAwareZkClient; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer; +import org.apache.helix.zookeeper.zkclient.exception.ZkException; +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.Op; +import org.apache.zookeeper.ZooDefs; + + +/** + * Fluent builder for an atomic ZooKeeper {@code multi()} transaction. Accumulates ops via + * {@link #set}/{@link #create}/{@link #delete}/{@link #check} and submits them as a single + * all-or-nothing batch on {@link #execute()}. + * + * <p>On atomic rollback (e.g. version mismatch, node missing, node already exists), {@link #execute()} + * throws the underlying {@link KeeperException} subtype ({@code BadVersionException}, + * {@code NoNodeException}, {@code NodeExistsException}, ...). Callers branch on the subtype to + * distinguish retryable concurrent-state changes from hard errors. Per-op offender info is reachable + * via {@link KeeperException#getResults()}. + * + * <p>Connectivity / session failures (timeout, interrupt, session expiry) are not atomic outcomes + * and propagate as the original {@link ZkException}. + * + * <p>Single-use: each instance can be executed at most once. Obtain a fresh builder per transaction + * (typically via {@code PinotHelixResourceManager.multiWriteZK()}). + */ +public final class ZkMultiWriteBuilder { Review Comment: I don't think this is threadsafe so we should document it as such ########## pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java: ########## @@ -2081,6 +2121,52 @@ public boolean setZKData(String path, ZNRecord record, int expectedVersion, int return _helixDataAccessor.getBaseDataAccessor().set(path, record, expectedVersion, accessOption); } + /** + * Returns a fresh {@link ZkMultiWriteBuilder} for submitting an atomic ZooKeeper {@code multi()} + * transaction (set / create / delete / version-check ops on any combination of znodes). Either + * every op commits or none do. + * <p>Eagerly validates the dedicated multi-write ZK client is available: requires the controller + * to have been constructed with a non-null zkAddress (via the {@link ControllerConf} constructor) + * and not yet {@link #stop()}-ped, otherwise throws {@link IllegalStateException}. + * <p>The builder's {@code execute()} throws {@link org.apache.zookeeper.KeeperException} on atomic + * rollback (the subtype identifies the cause: {@code BadVersionException}, {@code NoNodeException}, + * {@code NodeExistsException}, ...). Connectivity / session failures propagate as the original + * {@link org.apache.helix.zookeeper.zkclient.exception.ZkException}. + */ + public ZkMultiWriteBuilder multiWriteZK() { + return new ZkMultiWriteBuilder(getOrBuildMultiWriteZkClient()); Review Comment: Lets add a test on PinotHelixResourceManager that verifies ZkMultiWriteBuilder on multiWriteZK() for 2 segment metadata updates. This is a simple happy path test that verifies that client is built correctly and serialisation / deserialisation works as expected -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
