gnodet-bot commented on code in PR #27849:
URL: https://github.com/apache/camel/pull/27849#discussion_r4275575363
##########
components/camel-ai/camel-docling/src/test/java/org/apache/camel/component/docling/integration/OcrExtractionIT.java:
##########
@@ -180,10 +180,6 @@ void testOcrWithMultipleTextBlocks() throws Exception {
.isTrue();
// Docling recognizes the footer but classifies it as page furniture,
which the default body export excludes.
Review Comment:
⚠️ **Dangling comment:** The assertion this comment described was removed,
but the comment remains. Either restore the assertion or remove the comment.
```suggestion
```
##########
test-infra/camel-test-infra-docling/src/main/resources/org/apache/camel/test/infra/docling/services/container.properties:
##########
@@ -16,3 +16,4 @@
## ---------------------------------------------------------------------------
## Docling container configuration for test-infra
docling.container=quay.io/docling-project/docling-serve:v1.35.0
+docling.container.ppc64le=docling-serve:v1.35.0
Review Comment:
⚠️ **Missing registry/namespace:** `docling-serve:v1.35.0` has no registry
prefix, while the x86 image is `quay.io/docling-project/docling-serve:v1.35.0`.
If this is intentional (e.g. a locally-built image for ppc64le because upstream
doesn't publish one), please add a comment explaining that. Otherwise, if
`quay.io/docling-project/` does publish ppc64le images, use the fully-qualified
name to avoid relying on a locally available image:
```suggestion
docling.container.ppc64le=quay.io/docling-project/docling-serve:v1.35.0
```
##########
test-infra/camel-test-infra-docling/src/main/java/org/apache/camel/test/infra/docling/services/DoclingLocalContainerInfraService.java:
##########
@@ -68,6 +68,8 @@ public TestInfraDoclingContainer(boolean fixedPort) {
waitingFor(Wait.forListeningPorts(DOCLING_PORT))
.withStartupTimeout(Duration.ofMinutes(3L));
Review Comment:
💡 **Design:** `setProcessTimeout` was bumped to 300s and
`camel.failsafe.forkTimeout` to 1200s for ppc64le, but
`withStartupTimeout(Duration.ofMinutes(3L))` (3 min = 180s) was left unchanged.
On a slow ppc64le build, the container could hit the startup timeout before the
process timeout even comes into play. Consider bumping this — perhaps
architecture-conditionally:
```java
waitingFor(Wait.forListeningPorts(DOCLING_PORT))
.withStartupTimeout(Duration.ofMinutes(10L)); // ppc64le needs more
time
```
##########
test-infra/camel-test-infra-docling/src/main/java/org/apache/camel/test/infra/docling/services/DoclingLocalContainerInfraService.java:
##########
@@ -68,6 +68,8 @@ public TestInfraDoclingContainer(boolean fixedPort) {
waitingFor(Wait.forListeningPorts(DOCLING_PORT))
.withStartupTimeout(Duration.ofMinutes(3L));
+ withEnv("DOCLING_SERVE_MAX_SYNC_WAIT", "300");
Review Comment:
💡 **Design:** `DOCLING_SERVE_MAX_SYNC_WAIT=300` is set unconditionally for
all architectures. Is 300s the right value for x86 as well? The original
process timeout was 120s — if this change is ppc64le-specific, consider gating
it on the architecture property or documenting why a 5-minute wait is
appropriate everywhere.
##########
components/camel-ai/camel-docling/pom.xml:
##########
@@ -35,7 +35,7 @@
<properties>
<skipITs.s390x>true</skipITs.s390x>
- <skipITs.ppc64le>true</skipITs.ppc64le>
+ <camel.failsafe.forkTimeout>1200</camel.failsafe.forkTimeout>
Review Comment:
⚠️ **Style:** Tab indentation on this line while the rest of the file uses
4-space indentation. Please fix to match the surrounding style.
```suggestion
<camel.failsafe.forkTimeout>1200</camel.failsafe.forkTimeout>
```
--
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]
