keith-turner commented on code in PR #5416: URL: https://github.com/apache/accumulo/pull/5416#discussion_r2067441320
########## server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/CurrentLocationNotEqualToIterator.java: ########## @@ -0,0 +1,80 @@ +/* + * 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.server.metadata.iterators; + +import static org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.CurrentLocationColumnFamily; + +import java.io.IOException; +import java.util.Map; + +import org.apache.accumulo.core.client.IteratorSetting; +import org.apache.accumulo.core.data.Condition; +import org.apache.accumulo.core.data.Key; +import org.apache.accumulo.core.data.Value; +import org.apache.accumulo.core.iterators.IteratorEnvironment; +import org.apache.accumulo.core.iterators.SortedKeyValueIterator; +import org.apache.accumulo.core.metadata.TServerInstance; +import org.apache.accumulo.server.metadata.ConditionalTabletMutatorImpl; +import org.apache.hadoop.io.Text; + +public class CurrentLocationNotEqualToIterator extends ColumnFamilyTransformationIterator { + private static final String TSERVER_INSTANCE_OPTION = "tsi_option"; + private static final String NOT_EQUAL = "0"; + private static final String EQUAL = "1"; + private TServerInstance tsi; + + @Override + public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, + IteratorEnvironment env) throws IOException { + super.init(source, options, env); + tsi = new TServerInstance(options.get(TSERVER_INSTANCE_OPTION)); + } + + @Override + protected Value transform(SortedKeyValueIterator<Key,Value> source) throws IOException { + TServerInstance tsiSeen = null; + while (source.hasTop()) { + Value address = source.getTopValue(); + Text session = source.getTopKey().getColumnQualifier(); + tsiSeen = new TServerInstance(address, session); + if (tsiSeen.equals(tsi)) { + break; + } + source.next(); + } + + if (tsi.equals(tsiSeen)) { + return new Value(EQUAL); + } else { + return new Value(NOT_EQUAL); + } Review Comment: Maybe able to short circuit this. ```suggestion return new Value(EQUAL); } source.next(); } return new Value(NOT_EQUAL); ``` ########## server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java: ########## @@ -218,8 +218,6 @@ public LiveTServerSet(ServerContext context, Listener cback) { } public synchronized void startListeningForTabletServerChanges() { - scanServers(); Review Comment: The background thread will eventually call its unknown when that may happen. Could probably keep the immediate call and adjust the initial delay for the background thread. This makes reasoning about the state of this object after this method is called a bit easier. So something like the following. ```java public synchronized void startListeningForTabletServerChanges() { scanServers(); ThreadPools.watchCriticalScheduledTask(this.context.getScheduledExecutor() .scheduleWithFixedDelay(this::scanServers, 5000, 5000, TimeUnit.MILLISECONDS)); // Changed the initial delay from 0 to 5000 here } ``` ########## server/manager/src/main/java/org/apache/accumulo/manager/Manager.java: ########## @@ -666,16 +636,20 @@ public void run() { * balanceTablets() balances tables by DataLevel. Return the current set of migrations partitioned * by DataLevel */ - private static Map<DataLevel,Set<KeyExtent>> - partitionMigrations(final Set<KeyExtent> migrations) { + private Map<DataLevel,Set<KeyExtent>> partitionMigrations() { final Map<DataLevel,Set<KeyExtent>> partitionedMigrations = new EnumMap<>(DataLevel.class); - // populate to prevent NPE for (DataLevel dl : DataLevel.values()) { - partitionedMigrations.put(dl, new HashSet<>()); + Set<KeyExtent> extents = new HashSet<>(); + // prev row needed for the extent + try (var tabletsMetadata = getContext().getAmple().readTablets().forLevel(dl) + .fetch(TabletMetadata.ColumnType.PREV_ROW, TabletMetadata.ColumnType.MIGRATION).build()) { Review Comment: Not a change for this PR, but these scans for migrations could be optimized. Normally this will bring back a ton of prev row columns and a few migration columns. We could do a simple server side filter or something more complex that uses two iterators one that scans on the migration column and a second iterator that is used to get the prev row column only when the migration is seen. -- 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: notifications-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
