Copilot commented on code in PR #18502: URL: https://github.com/apache/pinot/pull/18502#discussion_r3244457388
########## pinot-controller/src/main/java/org/apache/pinot/controller/validation/DeprecatedFieldSpecChecker.java: ########## @@ -0,0 +1,112 @@ +/** + * 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.controller.validation; + +import java.util.HashSet; +import java.util.List; +import java.util.Properties; +import java.util.Set; +import org.apache.pinot.common.metrics.ControllerGauge; +import org.apache.pinot.common.metrics.ControllerMetrics; +import org.apache.pinot.controller.ControllerConf; +import org.apache.pinot.controller.LeadControllerManager; +import org.apache.pinot.controller.helix.core.PinotHelixResourceManager; +import org.apache.pinot.controller.helix.core.periodictask.ControllerPeriodicTask; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.spi.utils.builder.TableNameBuilder; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/// Periodic cluster health check that flags tables whose schema still uses the deprecated +/// [org.apache.pinot.spi.data.TimeFieldSpec]. +/// +/// For each table the current controller leads, it loads the table schema and: +/// +/// - logs a warning naming the table and schema if a `TimeFieldSpec` is present; +/// - emits a per-table gauge [ControllerGauge#TABLE_USES_DEPRECATED_TIME_FIELD_SPEC] (1 if affected, else 0); +/// - emits a global gauge [ControllerGauge#TABLES_WITH_DEPRECATED_TIME_FIELD_SPEC] with the count of affected +/// raw tables this controller saw on the latest run (hybrid tables count once). +/// +/// The count is naturally partitioned across lead controllers; summing the gauge across controllers gives the +/// cluster-wide total. Per-table gauges are cleared via [#nonLeaderCleanup] when this controller loses leadership +/// for a table so the partitioning stays sound. See https://github.com/apache/pinot/issues/2756 for the +/// deprecation plan. +public class DeprecatedFieldSpecChecker extends ControllerPeriodicTask<DeprecatedFieldSpecChecker.Context> { + private static final Logger LOGGER = LoggerFactory.getLogger(DeprecatedFieldSpecChecker.class); + + public DeprecatedFieldSpecChecker(ControllerConf config, PinotHelixResourceManager pinotHelixResourceManager, + LeadControllerManager leadControllerManager, ControllerMetrics controllerMetrics) { + super("DeprecatedFieldSpecChecker", config.getDeprecatedFieldSpecCheckerFrequencyInSeconds(), + config.getDeprecatedFieldSpecCheckerInitialDelayInSeconds(), pinotHelixResourceManager, leadControllerManager, + controllerMetrics); + } + + @Override + protected Context preprocess(Properties periodicTaskProperties) { + return new Context(); + } + + @Override + @SuppressWarnings("deprecation") + protected void processTable(String tableNameWithType, Context context) { + Schema schema = _pinotHelixResourceManager.getTableSchema(tableNameWithType); + if (schema == null) { + // Distinguish "not affected" (gauge=0) from "could not check" — do NOT overwrite the prior gauge here; + // a transient ZK miss should not silently flip a previously-flagged table to clean. + LOGGER.warn("No schema found for table {}; skipping deprecated-field check this run", tableNameWithType); + return; + } + if (schema.getTimeFieldSpec() != null) { + // Hybrid tables share a single raw schema; dedupe on raw table name so the global count is per-table. + if (context._affectedRawTables.add(TableNameBuilder.extractRawTableName(tableNameWithType))) { + LOGGER.warn("Table {} uses deprecated TimeFieldSpec (schema: {}); migrate to DateTimeFieldSpec. " + + "See https://github.com/apache/pinot/issues/2756";, tableNameWithType, schema.getSchemaName()); + } + _controllerMetrics.setValueOfTableGauge(tableNameWithType, + ControllerGauge.TABLE_USES_DEPRECATED_TIME_FIELD_SPEC, 1); + } else { + _controllerMetrics.setValueOfTableGauge(tableNameWithType, + ControllerGauge.TABLE_USES_DEPRECATED_TIME_FIELD_SPEC, 0); + } + } + + @Override + protected void postprocess(Context context) { + _controllerMetrics.setValueOfGlobalGauge(ControllerGauge.TABLES_WITH_DEPRECATED_TIME_FIELD_SPEC, + context._affectedRawTables.size()); + if (!context._affectedRawTables.isEmpty()) { + LOGGER.warn("{} table(s) led by this controller still use deprecated TimeFieldSpec", + context._affectedRawTables.size()); + } + } + + @Override + protected void nonLeaderCleanup(List<String> tableNamesWithType) { + // Once this controller is no longer leader for a table, drop the per-table gauge so summing the gauge across + // controllers does not double-count after a leadership shift. + for (String tableNameWithType : tableNamesWithType) { + _controllerMetrics.removeTableGauge(tableNameWithType, ControllerGauge.TABLE_USES_DEPRECATED_TIME_FIELD_SPEC); + } Review Comment: When this controller loses leadership for all tables, `ControllerPeriodicTask` skips `postprocess()` because there are no current leader tables, so `TABLES_WITH_DEPRECATED_TIME_FIELD_SPEC` retains its previous non-zero value. `nonLeaderCleanup()` removes the per-table gauges but should also reset/recompute the global gauge (for example, set it to 0 when no led tables remain) to avoid overcounting after leadership shifts. -- 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]
