suvodeep-pyne commented on code in PR #19737: URL: https://github.com/apache/pinot/pull/19737#discussion_r4213102932
########## pinot-server/src/test/java/org/apache/pinot/server/api/resources/ReingestionResourceTest.java: ########## @@ -0,0 +1,67 @@ +/** + * 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.server.api.resources; + +import org.testng.annotations.Test; + +import static org.apache.pinot.server.api.resources.ReingestionResource.CHECK_INTERVAL_MS; +import static org.apache.pinot.server.api.resources.ReingestionResource.waitForCondition; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + + +/// Tests how [ReingestionResource] waits for the re-ingestion consumption to complete. +public class ReingestionResourceTest { + + @Test + public void testWaitForConditionWithMaxTimeoutDoesNotTimeOutEarly() { + // The condition is false on the first check, so the wait must keep checking instead of timing out + long completionTimeMs = System.currentTimeMillis() + 50; + waitForCondition(() -> System.currentTimeMillis() >= completionTimeMs, 10, Long.MAX_VALUE); + } + + @Test + public void testWaitForConditionDetectsCompletionWithTimeoutShorterThanCheckInterval() { + // The condition becomes true after the first check. It must be checked again at the deadline instead of timing out + // after sleeping a full check interval. + long completionTimeMs = System.currentTimeMillis() + 100; + waitForCondition(() -> System.currentTimeMillis() >= completionTimeMs, CHECK_INTERVAL_MS, 1_000); Review Comment: Widened to 4,000 ms as suggested in 13ba976e3a. That's still below `CHECK_INTERVAL_MS`, so the test still catches the original bug. ########## pinot-server/src/main/java/org/apache/pinot/server/api/resources/ReingestionResource.java: ########## @@ -229,8 +235,12 @@ private void doReingestSegment(String realtimeTableName, SegmentZKMetadata segme String segmentName = segmentZKMetadata.getSegmentName(); try (StatelessRealtimeSegmentWriter writer = new StatelessRealtimeSegmentWriter(segmentZKMetadata, indexLoadingConfig, segmentBuildSemaphore)) { + // Read when the consumption starts, so that a job waiting for a re-ingestion thread uses the latest timeout + long consumptionTimeoutMs = _consumptionTimeout.getTimeoutMs(); Review Comment: Agreed it's only covered compositionally. Pinning it end to end needs a job that reaches `waitForCondition`, which means building a real `StatelessRealtimeSegmentWriter`: table config, schema and a stream consumer factory. That needs a stream-backed fixture the server tests don't have. As you suggested, I'm deferring it and relying on the pauseless commit-failure integration tests, which run the full repair path with the default timeout. The read is one line at consumption start, with a comment saying why. -- 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]
