hudi-agent commented on code in PR #18772:
URL: https://github.com/apache/hudi/pull/18772#discussion_r3260347863
##########
hudi-azure/src/test/java/org/apache/hudi/azure/transaction/lock/ITAzureStorageLockClientAzurite.java:
##########
@@ -63,16 +78,50 @@ public class ITAzureStorageLockClientAzurite {
private static final String ACCOUNT_KEY =
"Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==";
- @Container
- public static final GenericContainer<?> AZURITE =
- new GenericContainer<>(AZURITE_IMAGE)
- .withExposedPorts(10000)
- .withCommand("azurite-blob", "--blobHost", "0.0.0.0", "--blobPort",
"10000", "--loose")
-
.waitingFor(Wait.forListeningPort().withStartupTimeout(Duration.ofSeconds(60)));
+ // Manual container lifecycle (instead of @Testcontainers + @Container) so
the test class is
+ // skipped — not failed — when the image cannot be pulled. The
@Testcontainers extension
+ // surfaces image-pull errors as test errors, which is the wrong outcome for
an external
+ // infrastructure failure (e.g. MCR returning HTTP 403 "request blocked" via
Azure WAF).
+ private static GenericContainer<?> azurite;
+
+ @BeforeAll
+ static void startAzurite() {
+ GenericContainer<?> container = new GenericContainer<>(AZURITE_IMAGE)
+ .withExposedPorts(10000)
+ .withCommand("azurite-blob", "--blobHost", "0.0.0.0", "--blobPort",
"10000", "--loose")
+
.waitingFor(Wait.forListeningPort().withStartupTimeout(Duration.ofSeconds(60)));
+ try {
+ container.start();
+ azurite = container;
+ } catch (ContainerFetchException | ContainerLaunchException e) {
+ // Image pull / container start failure. Most commonly caused by MCR
being unreachable
+ // or rate-limited from CI runners. Skip the suite rather than failing
the build.
+ LOG.warn("Azurite container unavailable; skipping suite. Cause: {}",
e.toString());
Review Comment:
🤖 nit: `e.toString()` with a `{}` placeholder is non-idiomatic SLF4J — could
you use `e.getMessage()` here (and on the identical line in the second catch
block) if a stack trace isn't wanted, or drop the placeholder and pass `e` as
the trailing argument to let SLF4J handle it?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-azure/src/test/java/org/apache/hudi/azure/transaction/lock/ITAzureStorageLockClientAzurite.java:
##########
@@ -63,16 +78,50 @@ public class ITAzureStorageLockClientAzurite {
private static final String ACCOUNT_KEY =
"Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==";
- @Container
- public static final GenericContainer<?> AZURITE =
- new GenericContainer<>(AZURITE_IMAGE)
- .withExposedPorts(10000)
- .withCommand("azurite-blob", "--blobHost", "0.0.0.0", "--blobPort",
"10000", "--loose")
-
.waitingFor(Wait.forListeningPort().withStartupTimeout(Duration.ofSeconds(60)));
+ // Manual container lifecycle (instead of @Testcontainers + @Container) so
the test class is
+ // skipped — not failed — when the image cannot be pulled. The
@Testcontainers extension
+ // surfaces image-pull errors as test errors, which is the wrong outcome for
an external
+ // infrastructure failure (e.g. MCR returning HTTP 403 "request blocked" via
Azure WAF).
+ private static GenericContainer<?> azurite;
+
+ @BeforeAll
+ static void startAzurite() {
+ GenericContainer<?> container = new GenericContainer<>(AZURITE_IMAGE)
+ .withExposedPorts(10000)
+ .withCommand("azurite-blob", "--blobHost", "0.0.0.0", "--blobPort",
"10000", "--loose")
+
.waitingFor(Wait.forListeningPort().withStartupTimeout(Duration.ofSeconds(60)));
+ try {
+ container.start();
+ azurite = container;
+ } catch (ContainerFetchException | ContainerLaunchException e) {
+ // Image pull / container start failure. Most commonly caused by MCR
being unreachable
+ // or rate-limited from CI runners. Skip the suite rather than failing
the build.
+ LOG.warn("Azurite container unavailable; skipping suite. Cause: {}",
e.toString());
+ abort("Azurite container could not be started (image pull or launch
failed): " + e.getMessage());
+ } catch (RuntimeException e) {
+ // Testcontainers wraps a variety of Docker / network failures in plain
RuntimeExceptions
+ // during start(). Treat any "could not start container" failure as a
skip rather than
+ // an error for the same reason — these are infrastructure flakes, not
test failures.
+ // Re-throw if Docker itself is missing so the developer gets a clear
diagnosis locally.
+ if (e.getMessage() != null &&
e.getMessage().toLowerCase().contains("docker")
+ && e.getMessage().toLowerCase().contains("not")) {
Review Comment:
🤖 nit: `.contains("not")` is quite broad — a future reader won't immediately
know what Docker-absent message strings look like. It might be worth adding a
brief inline comment with an example of the actual message being matched (e.g.
`"Docker not found"` or `"Could not find a valid Docker environment"`), or
checking a more specific substring.
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
