gnodet-bot commented on code in PR #29420:
URL: https://github.com/apache/camel/pull/29420#discussion_r4506446657
##########
components/camel-aws/camel-aws2-athena/src/main/java/org/apache/camel/component/aws2/athena/Athena2Constants.java:
##########
@@ -62,6 +62,7 @@ Whether the response has more results (i.e., is truncated). \
Ensure this value ends with a forward slash ('/').""",
javaType = "String")
String OUTPUT_LOCATION = "CamelAwsAthenaOutputLocation";
+ String OUTPUT_LOCATION_OVERRIDE = "CamelAwsAthenaOutputLocationOverride";
Review Comment:
🐛 **Missing `@Metadata` annotation** — `OUTPUT_LOCATION_OVERRIDE` is added
without the `@Metadata` annotation that all adjacent constants carry. Without
it, Camel's codegen pipeline will not include this header in the generated
catalog, documentation, or IDE tooling. Add the annotation before this constant:
```suggestion
@Metadata(label = "startQueryExecution",
description = "The S3 location to use for storing query
results, overriding the endpoint-level outputLocation option.",
javaType = "String")
String OUTPUT_LOCATION_OVERRIDE = "CamelAwsAthenaOutputLocationOverride";
```
##########
components/camel-aws/camel-aws2-athena/src/main/java/org/apache/camel/component/aws2/athena/Athena2Producer.java:
##########
@@ -545,4 +532,4 @@ protected void doStop() throws Exception {
producerHealthCheck = null;
}
}
-}
+}
Review Comment:
🔧 **Missing newline at end of file** — please add a trailing newline to
`Athena2Producer.java`.
##########
components/camel-aws/camel-aws2-athena/src/main/java/org/apache/camel/component/aws2/athena/Athena2Producer.java:
##########
@@ -148,14 +154,13 @@ private void getQueryResults(AthenaClient athenaClient,
Exchange exchange) {
message.setBody(response);
} else if (outputType == Athena2OutputType.S3Pointer) {
GetQueryExecutionResponse response =
doGetQueryExecution(queryExecutionId, athenaClient);
- String outputLocation =
outputLocationOf(response.queryExecution());
message.setHeader(Athena2Constants.QUERY_EXECUTION_STATE,
response.queryExecution().status().state());
- setOutputLocationResult(exchange, outputLocation);
- message.setBody(outputLocation);
+ setOutputLocationResult(message, response);
+
message.setBody(message.getHeader(Athena2Constants.OUTPUT_LOCATION,
String.class));
Review Comment:
💡 **Unnecessary round-trip through the message header** — after
`setOutputLocationResult` sets the header, the body is read back via
`getHeader`. The old code kept a local variable and used it directly. Consider
using the local variable approach again for clarity:
```suggestion
setOutputLocationResult(message, response);
String outputLocation =
message.getHeader(Athena2Constants.OUTPUT_LOCATION, String.class);
message.setBody(outputLocation);
```
Or capture the value inside `setOutputLocationResult` and return it. As
written it works, but reads as if the body depends on the header being set by a
side effect.
##########
components/camel-aws/camel-aws2-athena/src/test/java/org/apache/camel/component/aws2/athena/AmazonAthenaClientMock.java:
##########
@@ -158,4 +160,4 @@ public void close() {
// noop
}
-}
+}
Review Comment:
🔧 **Missing newline at end of file** — please add a trailing newline to
`AmazonAthenaClientMock.java`.
##########
components/camel-aws/camel-aws2-athena/src/main/java/org/apache/camel/component/aws2/athena/Athena2Producer.java:
##########
@@ -53,10 +52,6 @@ public class Athena2Producer extends DefaultProducer {
private static final Logger LOG =
LoggerFactory.getLogger(Athena2Producer.class);
- // the output location and the next token that an operation set as its
result, kept in exchange properties, so a
- // later operation does not take them as its input: the output location of
a query is the file of its results, and
- // a next token is only valid for the operation (and query) that returned
it
- private static final String OUTPUT_LOCATION_RESULT =
"CamelAwsAthenaOutputLocationResult";
private static final String NEXT_TOKEN_RESULT =
"CamelAwsAthenaNextTokenResult";
Review Comment:
⚠️ **Orphaned constant** — `NEXT_TOKEN_RESULT` has lost its explanatory
comment. The 4-line block that explained the exchange-property pattern was
removed together with `OUTPUT_LOCATION_RESULT`, but `NEXT_TOKEN_RESULT` still
uses the same mechanism. Without the comment it is unclear why this is an
exchange property rather than a header, and what prevents a later operation
from consuming it. Please add a brief Javadoc or inline comment for
`NEXT_TOKEN_RESULT`.
--
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]
