Copilot commented on code in PR #2347:
URL: https://github.com/apache/kylin/pull/2347#discussion_r3781481473
##########
kystudio/src/components/setting/SettingModel/SettingModel.vue:
##########
@@ -141,6 +150,38 @@
</el-option>
</el-select>
</el-form-item>
+ <template v-if="step=='stepTwo'&&modelSettingForm.settingItem==='Auto
Segment Build'">
+ <el-form-item :label="$t('autoSegmentBuildTriggerTime')">
+ <el-time-picker
+ style="width: 180px;"
+ size="small"
+ format="HH:mm:ss"
+ value-format="HH:mm:ss"
+ v-model="modelSettingForm.autoSegmentBuild.trigger_time">
+ </el-time-picker>
+ </el-form-item>
+ <el-form-item :label="$t('autoSegmentBuildLogicalOffset')">
+ <el-input
v-model="modelSettingForm.autoSegmentBuild.logical_date_offset_days"
v-number="modelSettingForm.autoSegmentBuild.logical_date_offset_days"
class="retention-input"></el-input>
+ </el-form-item>
+ <el-form-item :label="$t('autoSegmentBuildRangeStart')">
+ <el-time-picker
+ style="width: 180px;"
+ size="small"
+ format="HH:mm:ss"
+ value-format="HH:mm:ss"
+
v-model="modelSettingForm.autoSegmentBuild.data_range_start_time">
+ </el-time-picker>
+ </el-form-item>
+ <el-form-item :label="$t('autoSegmentBuildRangeEnd')">
+ <el-time-picker
+ style="width: 180px;"
+ size="small"
+ format="HH:mm:ss"
+ value-format="HH:mm:ss"
+ v-model="modelSettingForm.autoSegmentBuild.data_range_end_time">
+ </el-time-picker>
Review Comment:
`data_range_end_time` uses `<el-time-picker>` with
`value-format="HH:mm:ss"`, but the default/allowed sentinel value `24:00:00`
(END_OF_DAY) is not a valid time for most time picker implementations and may
render as blank or throw parsing errors. This makes the default Auto Segment
Build form state and any backend-returned `24:00:00` values hard to edit in the
UI.
##########
src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelBuildService.java:
##########
@@ -281,9 +281,21 @@ public JobInfoResponse
incrementBuildSegmentsManually(String project, String mod
@Override
public JobInfoResponse
incrementBuildSegmentsManually(IncrementBuildSegmentParams params) throws
Exception {
+ return incrementBuildSegmentsInternal(params, getUsername(), true);
+ }
+
+ public JobInfoResponse
incrementBuildSegmentsByScheduler(IncrementBuildSegmentParams params, String
submitter)
+ throws Exception {
+ return incrementBuildSegmentsInternal(params, submitter, false);
+ }
+
+ private JobInfoResponse
incrementBuildSegmentsInternal(IncrementBuildSegmentParams params, String
submitter,
+ boolean checkPermission) throws Exception {
String project = params.getProject();
- aclEvaluate.checkProjectOperationPermission(project);
- checkModelPermission(project, params.getModelId());
+ if (checkPermission) {
+ aclEvaluate.checkProjectOperationPermission(project);
+ checkModelPermission(project, params.getModelId());
+ }
val modelManager = getManager(NDataModelManager.class, project);
Review Comment:
`incrementBuildSegmentsByScheduler(...)` allows callers to pass an arbitrary
`submitter`, and `createJobParam(...)` uses it as the job submitter/owner. If
`submitter` is blank/null (or accidentally user-controlled in the future), jobs
can be created with an empty or spoofed submitter. It’s safer to normalize it
to a default value before proceeding.
##########
src/data-loading-service/src/main/java/org/apache/kylin/rest/scheduler/AutoBuildSegmentScheduler.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * 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.kylin.rest.scheduler;
+
+import static
org.apache.kylin.metadata.model.AutoSegmentBuildConfig.END_OF_DAY;
+
+import java.time.Duration;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.time.ZoneId;
+import java.time.ZonedDateTime;
+import java.time.format.DateTimeFormatter;
+import java.time.format.DateTimeParseException;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.kylin.common.KylinConfig;
+import org.apache.kylin.common.util.Pair;
+import org.apache.kylin.guava30.shaded.common.annotations.VisibleForTesting;
+import org.apache.kylin.guava30.shaded.common.collect.Lists;
+import org.apache.kylin.job.execution.AbstractExecutable;
+import org.apache.kylin.job.execution.ExecutableManager;
+import org.apache.kylin.job.execution.ExecutableState;
+import org.apache.kylin.job.execution.JobTypeEnum;
+import org.apache.kylin.job.util.JobContextUtil;
+import org.apache.kylin.metadata.cube.model.NDataflowManager;
+import org.apache.kylin.metadata.model.AutoSegmentBuildConfig;
+import org.apache.kylin.metadata.model.NDataModel;
+import org.apache.kylin.metadata.model.PartitionDesc;
+import org.apache.kylin.metadata.project.NProjectManager;
+import org.apache.kylin.metadata.project.ProjectInstance;
+import org.apache.kylin.rest.service.ModelBuildService;
+import org.apache.kylin.rest.service.params.IncrementBuildSegmentParams;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Qualifier;
+import org.springframework.scheduling.annotation.Scheduled;
+import org.springframework.stereotype.Component;
+
+import lombok.val;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+@Component
+public class AutoBuildSegmentScheduler {
+ private static final Duration INITIAL_TRIGGER_LOOKBACK =
Duration.ofMinutes(1);
+ private static final DateTimeFormatter TIME_FORMATTER =
DateTimeFormatter.ofPattern("HH:mm:ss", Locale.ROOT);
+
+ @Autowired
+ @Qualifier("modelBuildService")
+ private ModelBuildService modelBuildService;
+
+ private final AtomicBoolean dispatching = new AtomicBoolean(false);
+ private final AtomicReference<Instant> lastDispatchTime = new
AtomicReference<>();
+
+ @Scheduled(cron = "${kylin.model.auto-segment-build.dispatcher-cron:*/30 *
* * * ?}")
+ public void schedulerAutoBuildSegment() {
+ val currentTime = Instant.now();
+ if
(!JobContextUtil.getJobContext(KylinConfig.getInstanceFromEnv()).getJobScheduler().isMaster())
{
+ lastDispatchTime.set(currentTime);
+ return;
+ }
+ if (!dispatching.compareAndSet(false, true)) {
+ log.warn("Skip auto build segment dispatch because the previous
dispatch is still running");
+ return;
+ }
+
+ val previousTime = getPreviousDispatchTime(currentTime);
+ try {
+ dispatch(previousTime, currentTime);
+ } finally {
+ lastDispatchTime.set(currentTime);
+ dispatching.set(false);
+ }
+ }
+
+ private Instant getPreviousDispatchTime(Instant currentTime) {
+ val previousTime = lastDispatchTime.get();
+ if (previousTime == null || previousTime.isAfter(currentTime)) {
+ return currentTime.minus(INITIAL_TRIGGER_LOOKBACK);
+ }
+ return previousTime;
+ }
+
+ @VisibleForTesting
+ void dispatch(Instant previousTime, Instant currentTime) {
+ val systemConfig = KylinConfig.readSystemKylinConfig();
+ val projectManager = NProjectManager.getInstance(systemConfig);
+ for (ProjectInstance project : projectManager.listAllProjects()) {
+ try {
+ dispatchProject(systemConfig, project, previousTime,
currentTime);
+ } catch (Exception e) {
+ log.error("Auto build segment dispatch failed for project:
{}", project.getName(), e);
+ }
+ }
+ }
+
+ private void dispatchProject(KylinConfig systemConfig, ProjectInstance
project, Instant previousTime,
+ Instant currentTime) {
+ val projectName = project.getName();
+ val zoneId = ZoneId.of(project.getConfig().getTimeZone());
+ val dataflowManager = NDataflowManager.getInstance(systemConfig,
projectName);
+ for (NDataModel model : dataflowManager.listOnlineDataModels()) {
+ try {
+ dispatchModel(projectName, model, zoneId, previousTime,
currentTime);
+ } catch (Exception e) {
+ log.error("Auto build segment dispatch failed, project: {},
model: {}", projectName, model.getUuid(),
+ e);
+ }
+ }
+ }
+
+ private void dispatchModel(String project, NDataModel model, ZoneId
zoneId, Instant previousTime,
+ Instant currentTime) {
+ val autoSegmentBuild = getEligibleConfig(model);
+ if (autoSegmentBuild == null) {
+ return;
+ }
+ if (StringUtils.isBlank(autoSegmentBuild.getTriggerTime())) {
+ log.warn("Skip auto build segment because trigger_time is blank,
project: {}, model: {}", project,
+ model.getUuid());
+ return;
+ }
Review Comment:
`dispatchModel()` only checks `trigger_time`, but `submitJob()` assumes
`logical_date_offset_days`, `data_range_start_time`, and `data_range_end_time`
are non-null/non-blank. If older metadata or manual edits leave these unset,
this will throw (NPE/parse errors) and spam logs on every dispatch interval.
Consider validating completeness up-front and skipping with a single warning.
##########
src/data-loading-service/src/main/java/org/apache/kylin/rest/scheduler/AutoBuildSegmentScheduler.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * 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.kylin.rest.scheduler;
+
+import static
org.apache.kylin.metadata.model.AutoSegmentBuildConfig.END_OF_DAY;
+
+import java.time.Duration;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
Review Comment:
`LocalDate` is imported but never referenced in this class, which can fail
the build if checkstyle/spotbugs flags unused imports.
##########
src/modeling-service/src/main/java/org/apache/kylin/rest/service/ModelService.java:
##########
@@ -3368,9 +3383,62 @@ public void
checkModelConfigParameters(ModelConfigRequest request) {
&& retentionRange.getRetentionRangeNumber() < 0) {
throw new KylinException(INVALID_PARAMETER,
MsgPicker.getMsg().getInvalidRetentionRangeConfig());
}
+ if (null != autoSegmentBuild && autoSegmentBuild.isEnabled()) {
+ checkAutoSegmentBuildConfig(project, modelId, autoSegmentBuild);
+ }
checkPropParameter(request);
}
+ private void checkAutoSegmentBuildConfig(String project, String modelId,
AutoSegmentBuildConfig autoSegmentBuild) {
+ if (StringUtils.isBlank(autoSegmentBuild.getTriggerTime())
+ ||
StringUtils.isBlank(autoSegmentBuild.getDataRangeStartTime())
+ || StringUtils.isBlank(autoSegmentBuild.getDataRangeEndTime())
+ || autoSegmentBuild.getLogicalDateOffsetDays() == null) {
+ throw new KylinException(INVALID_PARAMETER, "Invalid
auto_segment_build config.");
Review Comment:
When required `auto_segment_build` fields are missing, the exception message
is generic ("Invalid auto_segment_build config.") and doesn’t tell users which
parameters are required. Since other validation errors already reference
specific field names, consider listing the required fields here as well to make
the API error actionable.
--
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]
