joseluisll commented on PR #8753:
URL: https://github.com/apache/hadoop/pull/8753#issuecomment-5829474336
Thanks for picking this up. I reproduced the warning locally and I think the
patch is right in approach but over-applied by one field: as written it swaps
`SE_BAD_FIELD` for `SE_TRANSIENT_FIELD_NOT_RESTORED`, so `hadoop-common` still
reports one extant warning and trunk precommit stays red.
Three runs on a clean branch off trunk at `90f0d1da37c`, same command each
time, reading `hadoop-common-project/hadoop-common/target/spotbugsXml.xml` (not
just the exit code):
```
./mvnw -pl hadoop-common-project/hadoop-common -DskipTests -P'!native-win'
test-compile spotbugs:spotbugs
```
**1. Baseline, unmodified trunk — `total_bugs = 1`**
```
type='SE_BAD_FIELD' priority='2' rank='16' category='BAD_PRACTICE'
Class: org.apache.hadoop.mcp.McpHttpServlet
Field: name='requestHandler'
signature='Lorg/apache/hadoop/mcp/McpRequestHandler;'
Class org.apache.hadoop.mcp.McpHttpServlet defines non-transient
non-serializable instance field requestHandler
```
**2. This PR's diff (both fields `transient`) — `total_bugs = 1`**
```
type='SE_TRANSIENT_FIELD_NOT_RESTORED' priority='2' rank='16'
category='BAD_PRACTICE'
Class: org.apache.hadoop.mcp.McpHttpServlet
Field: name='objectMapper'
signature='Lcom/fasterxml/jackson/databind/ObjectMapper;'
The field org.apache.hadoop.mcp.McpHttpServlet.objectMapper is transient but
isn't set by deserialization
```
`SE_BAD_FIELD` is indeed gone, but the new warning lands on `objectMapper`.
**3. Only `requestHandler` marked `transient` — `total_bugs = 0`**
Clean. (`total_classes='2648'` on all three runs; `McpHttpServlet` is in the
analyzed set each time.)
The reason for the asymmetry: Jackson's `ObjectMapper` is itself
`Serializable`, so it never tripped `SE_BAD_FIELD` — which is why trunk reports
one warning rather than two. Marking it `transient` is what introduces
`SE_TRANSIENT_FIELD_NOT_RESTORED`. `McpRequestHandler` is not `Serializable`,
so `transient` there is the sanctioned fix and draws no complaint. Both fields
being `final` turned out not to matter to either detector.
So the suggestion is just to drop the `objectMapper` hunk and keep:
```java
private final ObjectMapper objectMapper;
private final transient McpRequestHandler requestHandler;
```
That also matches what other hadoop-common servlets do —
`ProfileServlet.process` and `JMXJsonServlet.mBeanServer` / `jsonFactory` use
`transient` for exactly the non-serializable collaborators and leave the rest
alone.
Happy to be wrong if your run shows something different — worth confirming,
since the Jenkins run on this PR only checks that the *patch* is clean, not
that the module reaches zero.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
