gemini-code-assist[bot] commented on code in PR #38047:
URL: https://github.com/apache/beam/pull/38047#discussion_r3187424144
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/io/FileIOTest.java:
##########
@@ -249,16 +249,25 @@ public void processElement(ProcessContext context,
@StateId("count") ValueState<
context.output(Objects.requireNonNull(context.element()).getValue());
CopyOption[] cpOptions = {StandardCopyOption.COPY_ATTRIBUTES};
- CopyOption[] updOptions = {StandardCopyOption.REPLACE_EXISTING};
+ CopyOption[] updOptions = {
+ StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES
+ };
final Path sourcePath = Paths.get(sourcePathStr);
final Path watchPath = Paths.get(watchPathStr);
if (0 == current) {
Thread.sleep(100);
+ // Ensure overwrite updates get a distinct mtime even when
COPY_ATTRIBUTES is enabled.
+ Files.setLastModifiedTime(
+ sourcePath.resolve("first"),
FileTime.fromMillis(System.currentTimeMillis() + 2000));
Review Comment:
Using a future timestamp (`System.currentTimeMillis() + 2000`) is a
workaround for filesystem resolution issues but can be fragile in environments
that validate file timestamps. Consider if a more deterministic way to ensure
increasing timestamps (e.g., using a base time and incrementing) would be more
reliable.
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/io/FileIOTest.java:
##########
@@ -249,16 +249,25 @@ public void processElement(ProcessContext context,
@StateId("count") ValueState<
context.output(Objects.requireNonNull(context.element()).getValue());
CopyOption[] cpOptions = {StandardCopyOption.COPY_ATTRIBUTES};
- CopyOption[] updOptions = {StandardCopyOption.REPLACE_EXISTING};
+ CopyOption[] updOptions = {
+ StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES
+ };
final Path sourcePath = Paths.get(sourcePathStr);
final Path watchPath = Paths.get(watchPathStr);
if (0 == current) {
Thread.sleep(100);
+ // Ensure overwrite updates get a distinct mtime even when
COPY_ATTRIBUTES is enabled.
+ Files.setLastModifiedTime(
+ sourcePath.resolve("first"),
FileTime.fromMillis(System.currentTimeMillis() + 2000));
Files.copy(sourcePath.resolve("first"), watchPath.resolve("first"),
updOptions);
Files.copy(sourcePath.resolve("second"), watchPath.resolve("second"),
cpOptions);
} else if (1 == current) {
Thread.sleep(100);
+ Files.setLastModifiedTime(
+ sourcePath.resolve("first"),
FileTime.fromMillis(System.currentTimeMillis() + 4000));
+ Files.setLastModifiedTime(
+ sourcePath.resolve("second"),
FileTime.fromMillis(System.currentTimeMillis() + 4000));
Review Comment:
Calling `System.currentTimeMillis()` multiple times for related updates can
lead to inconsistent timestamps if the clock ticks between calls. It's better
to capture the timestamp once. Also, the use of future offsets (e.g., `+ 4000`)
remains a potential source of flakiness on some platforms.
```suggestion
long now = System.currentTimeMillis();
Files.setLastModifiedTime(
sourcePath.resolve("first"), FileTime.fromMillis(now + 4000));
Files.setLastModifiedTime(
sourcePath.resolve("second"), FileTime.fromMillis(now + 4000));
```
--
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]
