wombatu-kun commented on code in PR #20397:
URL: https://github.com/apache/hudi/pull/20397#discussion_r4344329456
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/TestSparkSqlHudiPackageStructure.java:
##########
@@ -109,24 +106,18 @@ public void
testSparkSqlHudiScalaTestClassesInAllowedPackagesOnly() {
}
/**
- * Every Scala test class under {@link #BASE_PACKAGE} must be run by at
least one Azure step:
- * some step whose {@code -pl} builds the module holding the class must also
pass a
- * {@code -DwildcardSuites} prefix matching it. Otherwise the class silently
never runs on Azure.
- *
- * <p>The Azure jobs deliberately name leaf packages ({@code dml.others},
{@code dml.insert},
- * {@code dml.schema}) rather than the recursive {@code dml} parent, because
ScalaTest's
- * {@code -w} is a plain prefix match with no exclusion primitive: pointing
one job at
- * {@code ...hudi.dml} would re-run the whole {@code dml.insert} set that
already has its own
- * job. That split is what makes a newly added {@code dml.*} package start
out dark, so this
- * test is the guard for it - the other, non-recursive {@code
testSparkSqlHudi...} check above
- * lets {@code dml.*} through because it treats {@code dml} as one allowed
package.
+ * Guarantees that no Scala test class under {@link #BASE_PACKAGE} is left
out of the Spark 3.5
+ * run in {@link #SPARK35_WORKFLOW_FILE}: some {@code mvn test} step whose
{@code -pl} builds the
+ * module holding the class must also pass a {@code -DwildcardSuites} prefix
matching it. That
+ * workflow is read because its filters are spelled out in {@code env:}; a
class matched by no
+ * filter would never run on Spark 3.5.
*/
@Test
- public void testScalaTestPackagesAreRunByAnAzureStep() {
- List<AzureScalaTestStep> steps = readAzureScalaTestSteps();
+ public void testScalaTestPackagesAreRunBySpark35Step() {
Review Comment:
This check now reads only `spark35_ci.yml`, so a package that is in its
filters but missing from `bot.yml`'s passes both tests, and its suites never
run in the PR-time Spark 4.2 lanes. Could it parse `bot.yml` as well, skipping
commented-out lines, so the sync rule on `ALLOWED_PACKAGES` is enforced rather
than just documented?
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/TestSparkSqlHudiPackageStructure.java:
##########
@@ -190,94 +181,84 @@ private boolean runs(String className) {
}
/**
- * Parses the pipeline into the ScalaTest steps it defines. Resolves {@code
$(VAR)} tokens
- * through the {@code variables:} block back to the {@code parameters:} list
they join, so a
- * parameter list that no step actually expands contributes nothing.
+ * Parses the workflow into the ScalaTest steps it defines. {@code $NAME} /
{@code ${NAME}}
+ * tokens in {@code mvn test} lines are expanded from the workflow's {@code
env:} entries;
+ * entries whose value is a {@code ${{ ... }}} expression (e.g. matrix
values) stay unresolved.
*/
- private List<AzureScalaTestStep> readAzureScalaTestSteps() {
- List<String> lines = readAzurePipelineLines();
+ private List<ScalaTestStep> readScalaTestSteps() {
+ List<String> lines = readWorkflowLines();
- // parameters: <name> -> its list of values
- Map<String, List<String>> parameterLists = new HashMap<>();
- String currentParam = null;
+ Map<String, String> envValues = new HashMap<>();
for (String line : lines) {
- Matcher paramName = PARAM_NAME.matcher(line);
- if (paramName.matches()) {
- currentParam = paramName.group(1);
- parameterLists.put(currentParam, new ArrayList<>());
- continue;
- }
- Matcher listItem = LIST_ITEM.matcher(line);
- if (currentParam != null && listItem.matches()) {
- parameterLists.get(currentParam).add(listItem.group(1).trim());
+ Matcher entry = ENV_ENTRY.matcher(line);
+ if (entry.matches() && !entry.group(2).startsWith("${{")) {
+ envValues.put(entry.group(1), entry.group(2));
}
}
- // variables: <VAR> -> the parameter list it joins
- Map<String, List<String>> variableValues = new HashMap<>();
+ List<ScalaTestStep> steps = new ArrayList<>();
for (String line : lines) {
- Matcher variable = VARIABLE_JOIN.matcher(line);
- if (variable.matches()) {
- List<String> values = parameterLists.get(variable.group(2));
- if (values != null) {
- variableValues.put(variable.group(1), values);
- }
+ if (!line.contains("mvn test")) {
+ continue;
}
- }
-
- // steps: every line carrying both a wildcardSuites filter and a -pl
module list
- List<AzureScalaTestStep> steps = new ArrayList<>();
- for (String line : lines) {
- Matcher suites = WILDCARD_CLI_ARG.matcher(line);
- Matcher modules = PL_CLI_ARG.matcher(line);
+ String expanded = expandEnvVars(line, envValues);
+ Matcher suites = WILDCARD_CLI_ARG.matcher(expanded);
+ Matcher modules = PL_CLI_ARG.matcher(expanded);
if (!suites.find() || !modules.find()) {
continue;
}
- List<String> suitePrefixes = expand(suites.group(1),
variableValues).stream()
+ List<String> suitePrefixes = splitList(suites.group(1)).stream()
.filter(suite -> suite.startsWith("org."))
.collect(Collectors.toList());
if (suitePrefixes.isEmpty()) {
// e.g. the java-only steps, which pass -DwildcardSuites=skipScalaTests
continue;
}
- steps.add(new AzureScalaTestStep(suitePrefixes, expand(modules.group(1),
variableValues)));
+ steps.add(new ScalaTestStep(suitePrefixes, splitList(modules.group(1))));
}
return steps;
}
- /**
- * Splits a comma-separated Azure argument into its entries, replacing any
{@code $(VAR)} token
- * with the entries of the parameter list that variable joins.
- */
- private List<String> expand(String argument, Map<String, List<String>>
variableValues) {
- List<String> expanded = new ArrayList<>();
+ /** Replaces each shell variable reference whose name is in {@code
envValues} with its value. */
+ private String expandEnvVars(String line, Map<String, String> envValues) {
+ Matcher varRef = SHELL_VAR_REF.matcher(line);
+ StringBuffer expanded = new StringBuffer();
+ while (varRef.find()) {
+ String name = varRef.group(1) != null ? varRef.group(1) :
varRef.group(2);
+ String value = envValues.getOrDefault(name, varRef.group());
+ varRef.appendReplacement(expanded, Matcher.quoteReplacement(value));
+ }
+ varRef.appendTail(expanded);
+ return expanded.toString();
+ }
+
+ /** Splits a comma-separated argument into its non-empty entries. */
+ private List<String> splitList(String argument) {
+ List<String> entries = new ArrayList<>();
for (String token : argument.split(",")) {
String trimmed = token.trim();
- Matcher varRef = VAR_REF.matcher(trimmed);
- if (varRef.matches()) {
- expanded.addAll(variableValues.getOrDefault(varRef.group(1), new
ArrayList<>()));
- } else if (!trimmed.isEmpty()) {
- expanded.add(trimmed);
+ if (!trimmed.isEmpty()) {
+ entries.add(trimmed);
}
}
- return expanded;
+ return entries;
}
- private List<String> readAzurePipelineLines() {
+ private List<String> readWorkflowLines() {
File projectRoot = findProjectRoot();
if (projectRoot == null) {
fail("Could not locate project root directory");
return new ArrayList<>();
}
- File pipeline = new File(projectRoot, AZURE_PIPELINE_FILE);
- if (!pipeline.exists()) {
- fail("Could not locate " + AZURE_PIPELINE_FILE + " at " +
pipeline.getAbsolutePath());
+ File workflow = new File(projectRoot, SPARK35_WORKFLOW_FILE);
+ if (!workflow.exists()) {
+ fail("Could not locate " + SPARK35_WORKFLOW_FILE + " at " +
workflow.getAbsolutePath());
Review Comment:
The source release excludes `.github`
(`scripts/release/create_source_directory.sh`), so running the hudi-spark unit
tests from the released sources now fails here with "Could not locate
.github/workflows/spark35_ci.yml". Could this skip via `assumeTrue` when
`.github` itself is absent, and still fail when only the workflow file is
missing?
##########
.github/workflows/spark35_ci.yml:
##########
@@ -0,0 +1,255 @@
+# 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.
+
+name: Spark 3.5 CI
+
+on:
+ push:
+ branches:
+ - master
+ pull_request:
+ branches:
+ - master
+ paths:
+ - '.github/workflows/spark35_ci.yml'
Review Comment:
No PR-time lane runs tests against `hudi-spark3.5.x` or `hudi-spark3-common`
any more, so a PR that changes only Spark 3 shim code is first tested after it
merges. Could `pull_request.paths` also list those two modules, which only
about one merge a week touches?
##########
.github/workflows/spark35_ci.yml:
##########
@@ -0,0 +1,255 @@
+# 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.
+
+name: Spark 3.5 CI
+
+on:
+ push:
+ branches:
+ - master
+ pull_request:
+ branches:
+ - master
+ paths:
+ - '.github/workflows/spark35_ci.yml'
+
+concurrency:
+ group: ${{ github.workflow }}-${{ github.ref }}
+ cancel-in-progress: ${{ !contains(github.ref, 'master') }}
Review Comment:
The default `queue: single` keeps at most one pending run, so each newer
master push cancels the one already waiting, and going by last month's merge
times about one master commit in six would never get a Spark 3.5 run. Is that
batching intended, or should master runs queue up (`queue: max`, which cannot
be combined with `cancel-in-progress: true`)?
--
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]
