swaminathanmanish commented on code in PR #14300: URL: https://github.com/apache/pinot/pull/14300#discussion_r1842042118
########## pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentrefresh/SegmentRefreshTaskExecutor.java: ########## @@ -0,0 +1,200 @@ +/** + * 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.plugin.minion.tasks.segmentrefresh; + +import java.io.File; +import java.util.Collections; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import org.apache.pinot.common.metadata.segment.SegmentZKMetadataCustomMapModifier; +import org.apache.pinot.core.common.MinionConstants; +import org.apache.pinot.core.minion.PinotTaskConfig; +import org.apache.pinot.plugin.minion.tasks.BaseSingleSegmentConversionExecutor; +import org.apache.pinot.plugin.minion.tasks.SegmentConversionResult; +import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader; +import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl; +import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig; +import org.apache.pinot.segment.local.segment.readers.PinotSegmentRecordReader; +import org.apache.pinot.segment.spi.ColumnMetadata; +import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig; +import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl; +import org.apache.pinot.segment.spi.loader.SegmentDirectoryLoaderContext; +import org.apache.pinot.segment.spi.loader.SegmentDirectoryLoaderRegistry; +import org.apache.pinot.segment.spi.store.SegmentDirectory; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.data.FieldSpec; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.spi.env.PinotConfiguration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +public class SegmentRefreshTaskExecutor extends BaseSingleSegmentConversionExecutor { + private static final Logger LOGGER = LoggerFactory.getLogger(SegmentRefreshTaskGenerator.class); + + private long _taskStartTime; + + /** + * The code here currently covers segmentRefresh for the following cases: + * 1. Process newly added columns. + * 2. Addition/removal of indexes. + * 3. Compatible datatype change for existing columns + */ + @Override + protected SegmentConversionResult convert(PinotTaskConfig pinotTaskConfig, File indexDir, File workingDir) + throws Exception { + _eventObserver.notifyProgress(pinotTaskConfig, "Refreshing segment: " + indexDir); + + // We set _taskStartTime before fetching the tableConfig. Task Generation relies on tableConfig/Schema updates + // happening after the last processed time. So we explicity use the timestamp before fetching tableConfig as the + // processedTime. + _taskStartTime = System.currentTimeMillis(); Review Comment: My concerns are, we are unnecessarily doing the following 1. Creating a large number of subtasks when refresh is not required 2. BaseSingleSegmentConversionExecutor modifies Zk metadata and even uploads the segment, which is not necessary when refresh for that segment is not required. Segment requires no changes. We would end up updating all segments via controller which is not required. @vrajat is working on the server side API to detect diverged segments. This will reduce the payload by not shipping the segment metadata/but only sending the segment names that require refresh. The call to the server will only happen during task scheduled trigger (order of minutes/hours) which in my opinion should not put load on the server. Until then we can use getSegmentMetadata call on the server? https://github.com/apache/pinot/issues/14450 We cannot get this on the server? `Moreover, doing it at minion gives us the flexibility of also support compression type changes on segments (where we have to initialize the forward index reader to get the compression info)` -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
