Eliaaazzz commented on code in PR #39916:
URL: https://github.com/apache/beam/pull/39916#discussion_r3886003108
##########
runners/spark/job-server/spark_job_server.gradle:
##########
@@ -232,6 +232,8 @@ def portableValidatesRunnerTask(String name, boolean
streaming, boolean docker,
"beam.spark.test.reuseSparkContext": "false",
"spark.ui.enabled": "false",
"spark.ui.showConsoleProgress": "false",
+ // For Windows OS: see
https://cwiki.apache.org/confluence/spaces/HADOOP2/pages/120730292/WindowsProblems
+ // 'hadoop.home.dir':
"${rootDir.absolutePath}\\build\\hadoop-home",
Review Comment:
nit: is the commented-out property intentional? The line carries a tab
indent, the only one in this file. If it is meant as a breadcrumb for running
the Spark suites on Windows, the URL comment alone might be enough.
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingService.java:
##########
@@ -510,6 +515,9 @@ private String createFilename(String environment,
RunnerApi.ArtifactInformation
// all path separators.
List<String> components =
Splitter.onPattern("[^A-Za-z-_.]]").splitToList(path);
String base = components.get(components.size() - 1);
+ if (IS_OS_WINDOWS) {
+ environment =
WINDOWS_INVALID_CHARS.matcher(environment).replaceAll("_");
Review Comment:
The environment change addresses the reported case and preserves the Unicode
behavior we discussed on #39363.
`base` on the line above can still break the filename on Windows, the same
gap I noted on #39363: the splitter has an extra `]` after the character class,
so it only matches a disallowed character followed by a literal `]`. For an
ordinary absolute path it therefore does not split, and `base` stays the whole
`path` (the stray `]` is also written up in the last section of #39336). When a
file-type artifact with a role other than `staging_to` carries an absolute
client-side path, that path lands in the filename. One concrete in-tree
producer is Python's `_create_file_pip_requirements_artifact` in `stager.py`,
fed from `PyPIArtifactRegistry` through `python_sdk_dependencies`.
Verified at this head on Windows 11 by adding this to
`ArtifactStagingServiceTest`:
```java
@Test(timeout = 45_000)
public void testStageFileArtifactWithAbsolutePath() throws Exception {
java.io.File source = tempFolder.newFile("real-artifact.bin");
java.nio.file.Files.write(
source.toPath(),
"payload".getBytes(java.nio.charset.StandardCharsets.UTF_8));
RunnerApi.ArtifactInformation fileArtifact =
RunnerApi.ArtifactInformation.newBuilder()
.setTypeUrn(ArtifactRetrievalService.FILE_ARTIFACT_URN)
.setTypePayload(
RunnerApi.ArtifactFilePayload.newBuilder()
.setPath(source.getAbsolutePath())
.build()
.toByteString())
.setRoleUrn("beam:artifact:role:pip_requirements_file:v1")
.build();
stagingService.registerJob("fileToken", ImmutableMap.of("env",
ImmutableList.of(fileArtifact)));
ArtifactStagingService.offer(retrievalService, stagingStub, "fileToken");
assertEquals(1, stagingService.getStagedArtifacts("fileToken").size());
}
```
It fails with `ExecutionException: StatusRuntimeException: UNKNOWN` out of
`offer`, and the server side logs
```
InvalidPathException: Illegal char <:> at index 7:
1-env-C:\Users\...\real-artifact.bin
```
so `base` is the entire absolute path.
A one-line option: repair the splitter to `"[^A-Za-z0-9-_.]"`. Java parses
the dash after `0-9` as a literal there, and the pattern splits on both `\` and
`/`, so `base` drops the leading directories. With that change the repro above
and the full `:runners:java-fn-execution:test` suite pass on my machine. The
allowlist is ASCII-only, the tradeoff we discussed on #39363, though here it
only trims `base` while the unique id prefix keeps names distinct. Also
completely fine to keep this PR scoped to the environment id and handle `base`
separately, whichever you prefer.
--
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]
