dlmarion commented on code in PR #5451: URL: https://github.com/apache/accumulo/pull/5451#discussion_r2038334227
########## core/src/main/java/org/apache/accumulo/core/util/tables/TableMapping.java: ########## @@ -0,0 +1,195 @@ +/* + * 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.util.tables; + +import static java.util.Collections.emptySortedMap; +import static java.util.Objects.requireNonNull; +import static org.apache.accumulo.core.clientImpl.NamespaceMapping.deserializeMap; +import static org.apache.accumulo.core.clientImpl.NamespaceMapping.serializeMap; + +import java.util.Map; +import java.util.Objects; +import java.util.SortedMap; +import java.util.stream.Stream; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.clientImpl.AcceptableThriftTableOperationException; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.clientImpl.Namespace; +import org.apache.accumulo.core.clientImpl.thrift.TableOperation; +import org.apache.accumulo.core.clientImpl.thrift.TableOperationExceptionType; +import org.apache.accumulo.core.data.NamespaceId; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter; +import org.apache.accumulo.core.metadata.AccumuloTable; +import org.apache.accumulo.core.zookeeper.ZcStat; +import org.apache.accumulo.core.zookeeper.ZooCache; +import org.apache.zookeeper.KeeperException; + +import com.google.common.collect.ImmutableSortedMap; + +public class TableMapping { + + private final ClientContext context; + private final NamespaceId namespaceId; + private volatile SortedMap<TableId,String> currentTableMap = emptySortedMap(); + private volatile SortedMap<String,TableId> currentTableReverseMap = emptySortedMap(); + private volatile long lastMzxid; + + public TableMapping(ClientContext context, NamespaceId namespaceId) { + this.context = context; + this.namespaceId = namespaceId; + } + + public void put(final ClientContext context, TableId tableId, String tableName, + TableOperation operation) + throws InterruptedException, KeeperException, AcceptableThriftTableOperationException { + var zoo = context.getZooSession().asReaderWriter(); + Stream.of(zoo, tableId, namespaceId, tableName).forEach(Objects::requireNonNull); + String zTableMapPath = getZTableMapPath(namespaceId); + if (isBuiltInZKTable(tableId)) { + throw new AssertionError("Putting built-in tables in map should not be possible after init"); + } Review Comment: I think you can move these checks up to the beginning of the method body. No need to create the ZooReaderWriter or get the path in ZooKeeper if the input arguments are invalid. ########## core/src/main/java/org/apache/accumulo/core/util/tables/TableMapping.java: ########## @@ -0,0 +1,195 @@ +/* + * 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.util.tables; + +import static java.util.Collections.emptySortedMap; +import static java.util.Objects.requireNonNull; +import static org.apache.accumulo.core.clientImpl.NamespaceMapping.deserializeMap; +import static org.apache.accumulo.core.clientImpl.NamespaceMapping.serializeMap; + +import java.util.Map; +import java.util.Objects; +import java.util.SortedMap; +import java.util.stream.Stream; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.clientImpl.AcceptableThriftTableOperationException; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.clientImpl.Namespace; +import org.apache.accumulo.core.clientImpl.thrift.TableOperation; +import org.apache.accumulo.core.clientImpl.thrift.TableOperationExceptionType; +import org.apache.accumulo.core.data.NamespaceId; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter; +import org.apache.accumulo.core.metadata.AccumuloTable; +import org.apache.accumulo.core.zookeeper.ZcStat; +import org.apache.accumulo.core.zookeeper.ZooCache; +import org.apache.zookeeper.KeeperException; + +import com.google.common.collect.ImmutableSortedMap; + +public class TableMapping { + + private final ClientContext context; + private final NamespaceId namespaceId; + private volatile SortedMap<TableId,String> currentTableMap = emptySortedMap(); + private volatile SortedMap<String,TableId> currentTableReverseMap = emptySortedMap(); + private volatile long lastMzxid; + + public TableMapping(ClientContext context, NamespaceId namespaceId) { + this.context = context; + this.namespaceId = namespaceId; + } + + public void put(final ClientContext context, TableId tableId, String tableName, + TableOperation operation) + throws InterruptedException, KeeperException, AcceptableThriftTableOperationException { + var zoo = context.getZooSession().asReaderWriter(); + Stream.of(zoo, tableId, namespaceId, tableName).forEach(Objects::requireNonNull); Review Comment: I'm sorry, this is a pet-peeve of mine. --> gets back on anti-Stream soap box There's a tendency to use Streams to improve readability, reduce lines of code, etc. What appears to be a simple one line utility can be a performance killer. This isn't a critical piece of code, but if we start using Streams everywhere, it's going to add up. [Objects.requireNonNull](https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/util/Objects.java#L219) simply throws an NPE if the object is null. This one line of code will perform better if it were 4 lines of code that just called `Objects.requireNonNull`. Here's what this innocuous `Stream.of` does: 1. [First](https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/util/stream/Stream.java#L1187) it calls `Arrays.stream` on the values that you pass it. 2. [Arrays.stream](https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/util/Arrays.java#L5633) calls [spliterator](https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/util/Arrays.java#L5482) which then calls [Spliterators.spliterator](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/Spliterators.java#L163) which ends up calling `Objects.requireNonNull` on your array of objects, checks the bounds, then allocates a new [ArraySpliterator](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/Spliterators.java#L939). Then `Arrays.stream` calls `StreamSupport.stream` passing the `ArraySpliterator` that we just created. 3. [StreamSupport.stream](https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/util/stream/StreamSupport.java#L67) checks that the `ArraySpliterator` is not null, then creates a new `ReferencePipeline.Head` object. 4. [ReferencePipeline.Head](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/ReferencePipeline.java#L771) also initializes its parent classes [ReferencePipeline](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/ReferencePipeline.java#L74) and [AbstractPipeline](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/AbstractPipeline.java#L153) 5. Now that we have the Stream created, then we call [forEach](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/ReferencePipeline.java#L631). 6. `forEach` calls [ForEachOps.makeRef](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/ForEachOps.java#L72) which checks that the `Objects.requireNonNull` action we are passing is not null and then it creates yet another object [ForEachOp.OfRef](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/ForEachOps.java#L176). Next `forEach` calls [evaluate](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/AbstractPipeline.java#L257) passing in the newly created `ForEachOp.OfRef` object. 7. `evaluate` calls [sourceSpliterator](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/AbstractPipeline.java#L458) and I stopped reading at this point. After calling `sourceSpliterator`, `evaluate` then calls [ForEachOps.evaluateSequential](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/ForEachOps.java#L151). It doesn't end there, I just tired of tracing through the code. Next it uses some helper object and calls `wrapAndCopyInto`. You just wanted to check whether or not it was null, right? --> end rant -- 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]
