peterxcli commented on code in PR #11056: URL: https://github.com/apache/ozone/pull/11056#discussion_r3880794242
########## hadoop-ozone/dev-support/checks/errorprone.sh: ########## @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +#checks:basic + +set -u -o pipefail + +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" +cd "$DIR/../../.." || exit 1 + +REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/errorprone"} +REPORT_FILE="$REPORT_DIR/summary.txt" +OUTPUT_LOG=$(mktemp) + +MAVEN_OPTIONS='-B -fae --no-transfer-progress -Perrorprone -DskipDocs -DskipRecon -DskipShade' +MAVEN_DIAGNOSTIC_PATTERN='^\[(ERROR|WARNING)\] .*:\[[0-9]+,[0-9]+\] \[[^]]+\]' +JAVAC_DIAGNOSTIC_PATTERN='^.*:[0-9]+: (error|warning): \[[^]]+\]' +MAVEN_ERROR_PATTERN='^\[ERROR\] .*:\[[0-9]+,[0-9]+\] \[[^]]+\]' +JAVAC_ERROR_PATTERN='^.*:[0-9]+: error: \[[^]]+\]' +ERROR_PATTERN="${MAVEN_ERROR_PATTERN}|${JAVAC_ERROR_PATTERN}" + +declare -i rc + +trap 'rm -f "$OUTPUT_LOG"' EXIT + +#shellcheck disable=SC2086 +mvn $MAVEN_OPTIONS clean test-compile "$@" 2>&1 | tee "$OUTPUT_LOG" +rc=$? + +mkdir -p "$REPORT_DIR" +mv "$OUTPUT_LOG" "${REPORT_DIR}/output.log" +trap - EXIT + +grep -E "${MAVEN_DIAGNOSTIC_PATTERN}|${JAVAC_DIAGNOSTIC_PATTERN}" "${REPORT_DIR}/output.log" \ + | awk '!seen[$0]++' > "$REPORT_FILE" + +grep -E -c "$ERROR_PATTERN" "$REPORT_FILE" > "$REPORT_DIR/failures" || true + +if grep -q -E "$ERROR_PATTERN" "$REPORT_FILE"; then + { + printf '### Error Prone errors\n\n```text\n' + grep -E "$ERROR_PATTERN" "$REPORT_FILE" + printf '```\n' + } > "$REPORT_DIR/summary.md" +else + rm -f "$REPORT_DIR/summary.md" +fi + +exit ${rc} Review Comment: This is the one I'd like to see addressed before merge. Every other Maven-based check (`pmd.sh`, `findbugs.sh`, `checkstyle.sh`, `rat.sh`) ends with `source "${DIR}/_post_process.sh"`; this one hand-rolls the same logic and loses a case that `_post_process.sh` handles: ``` # script failed, but report file is empty (does not reflect failure) if [[ ${rc} -ne 0 ]] && [[ ! -s "${REPORT_FILE}" ]]; then echo "Unknown failure, check output.log" > "${REPORT_FILE}" fi ``` Concretely: if Maven fails for a reason that isn't an Error Prone ERROR — a plain compile error (`cannot find symbol` has no `[BugPattern]` tag, so it matches neither `MAVEN_ERROR_PATTERN` nor `JAVAC_ERROR_PATTERN`), a dependency resolution failure, or an OOM in the forked compiler — then: - `failures` is written as `0`, - `summary.md` is deleted, - `summary.txt` contains only unrelated Error Prone WARNINGS (or is empty), - but the job exits non-zero. The `Summary of failures` step in `check.yml` then runs `_summary.sh` on that file and prints a wall of warnings, or nothing at all, for a red job — which is exactly the confusing state `_post_process.sh` exists to avoid. This isn't hypothetical for a check that recompiles the whole reactor from clean. The cheap fix is to make the split explicit so the shared helper still works: put only the *errors* in `summary.txt` and the warnings somewhere else, e.g. ```bash grep -E "${MAVEN_DIAGNOSTIC_PATTERN}|${JAVAC_DIAGNOSTIC_PATTERN}" "${REPORT_DIR}/output.log" \ | awk '!seen[$0]++' > "${REPORT_DIR}/diagnostics.txt" grep -E "$ERROR_PATTERN" "${REPORT_DIR}/diagnostics.txt" > "$REPORT_FILE" || true source "${DIR}/_post_process.sh" ``` That keeps the full warning list in the artifact, gives you the "unknown failure" fallback and the `ITERATIONS` handling for free, and drops ~15 lines. `dev-support/ci/errorprone.bats` would need its expectations moved from `summary.txt` to `diagnostics.txt`; it'd also be worth adding a third case there for "Maven fails with no Error Prone diagnostics", since that's the path that's currently unguarded. ########## hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocolPB/SCMSecurityProtocolClientSideTranslatorPB.java: ########## @@ -110,7 +110,9 @@ private SCMSecurityResponse handleError(SCMSecurityResponse resp) throws SCMSecurityException { if (resp.getStatus() != SCMSecurityProtocolProtos.Status.OK) { throw new SCMSecurityException(resp.getMessage(), - SCMSecurityException.ErrorCode.values()[resp.getStatus().ordinal()]); + resp.getStatus() == SCMSecurityProtocolProtos.Status.GET_ROOT_CA_CERTIFICATE_FAILED + ? SCMSecurityException.ErrorCode.GET_ROOT_CA_CERT_FAILED + : SCMSecurityException.ErrorCode.valueOf(resp.getStatus().name())); Review Comment: Of the four `values()[ordinal()]` -> `valueOf(name())` conversions in this PR, this is the only one not backed by a name-parity test — and it's also the only one where the enums genuinely differ, which is why the ternary above is needed. `ScmServerSecurityProtocol.proto` also declares: ```proto //17 are removed from the code, kept here to preserve the numbers. REVOKE_CERTIFICATE_FAILED = 17; ``` There is no `REVOKE_CERTIFICATE_FAILED` in `SCMSecurityException.ErrorCode`, so if an SCM ever returns it, `valueOf` throws `IllegalArgumentException` out of a method declared `throws SCMSecurityException`. Not a regression — `values()[16]` was an `ArrayIndexOutOfBoundsException` — so I'm not asking you to fix it here. What I would ask for: a `TestSCMSecurityExceptionErrorCodes` alongside the existing `TestSCMExceptionResultCodes` / `TestResultCodes`, asserting that every `Status` name maps to an `ErrorCode` (with `GET_ROOT_CA_CERTIFICATE_FAILED` and `REVOKE_CERTIFICATE_FAILED` as explicit exceptions). Without it, the next `Status` added to the proto compiles fine and blows up at runtime on the client, whereas the other two enums fail the build. A short comment on the ternary explaining *why* these two names diverge would help too — it reads like an arbitrary special case otherwise. ########## hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/CompactionDag.java: ########## @@ -83,7 +83,7 @@ public Set<String> pruneNodesFromDag(Set<CompactionNode> nodesToRemove) { Set<String> sstFilesPruned = pruneForwardDag(forwardCompactionDAG, nodesToRemove); // Remove SST file nodes from compactionNodeMap too, // since those nodes won't be needed after clean up. - nodesToRemove.forEach(compactionNodeMap::remove); + nodesToRemove.forEach(node -> compactionNodeMap.remove(node.getFileName())); Review Comment: This is the most consequential fix in the PR and it has no test. `compactionNodeMap` is `Map<String, CompactionNode>`, and `Map#remove(Object)` happily accepted a `CompactionNode`, so the old `compactionNodeMap::remove` was a guaranteed no-op — pruned SST nodes were never evicted. After this change they really are removed, which changes both the DAG contents seen by `getSSTDiffListWithFullPath` and the memory retained across pruning cycles. `TestRocksDBCheckpointDiffer` only reads `compactionNodeMap` from helper methods (around lines 1242 and 1302); nothing asserts what it holds after `pruneNodesFromDag`. Per the repo's testing guidance ("New behavior and bug fixes should come with tests"), could you add a small unit test that builds a DAG, prunes a level, and asserts the pruned file names are gone from `compactionNodeMap` while the survivors remain? You already added `testGetContainersIncludesUniquePipelines` for the Recon fix — this one deserves the same treatment, and it's the one with actual snapshot-diff correctness impact. ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconContainerMetadataManagerImpl.java: ########## @@ -514,11 +514,14 @@ private List<Pipeline> getPipelines(ContainerKeyPrefix containerKeyPrefix) } List<Pipeline> pipelines = new ArrayList<>(); if (null != omKeyInfo) { - omKeyInfo.getKeyLocationVersions().stream().map( - omKeyLocationInfoGroup -> - omKeyLocationInfoGroup.createLocationList() - .stream().map(omKeyLocationInfo -> pipelines.add( - omKeyLocationInfo.getPipeline()))); + omKeyInfo.getKeyLocationVersions().forEach(omKeyLocationInfoGroup -> Review Comment: Good catch — the old chain had no terminal operation, so `pipelines` was always empty and `ContainerMetadata#getPipelines()` always returned `[]`. Thanks for adding `testGetContainersIncludesUniquePipelines`. Worth flagging explicitly though: this changes a user-visible Recon REST payload. Every `/api/v1/containers` response now carries a populated `pipelines` array where it previously carried an empty one, and `createLocationList()` now actually runs per location group (the lazy stream never invoked it). Anything written against the always-empty field — the Recon UI, downstream tooling — sees a different response. Could you call this out in the PR description, and ideally a release note? It's the sort of change that's easy to miss when it's the 40th commit in a static-analysis PR. If you'd rather keep the blast radius small, splitting this one hunk plus its test into its own Jira would also be reasonable. ########## hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java: ########## @@ -493,18 +493,24 @@ public void testHSyncOpenKeyCommitAfterExpiry() throws Exception { try (FSDataOutputStream os1 = fs.create(key2, true)) { os1.write(1); // There should be 2 key in openFileTable - assertThat(2 == getOpenKeyInfo(BUCKET_LAYOUT).size()); - // One key will be in fileTable as hsynced - assertThat(1 == getKeyInfo(BUCKET_LAYOUT).size()); + assertThat(getOpenKeyInfo(BUCKET_LAYOUT)).hasSize(2); Review Comment: `getOpenKeyInfo()` iterates the OM's entire `openFileTable`, not just this test's keys — and `TestHSync` runs ~28 methods against one `@BeforeAll` `MiniOzoneCluster` with `@TestMethodOrder(OrderAnnotation.class)`, so the un-annotated methods run in discovery order. Turning the old no-op `assertThat(2 == ...size())` into a real `hasSize(2)` therefore makes this assertion depend on no other method having left an hsync'd-but-unclosed key behind. Same for `hasSize(1)` at line 588. Green today, but it's the kind of assertion that gets flaky when someone adds a test above it, and the failure message (`expected size 2 but was 3`) points nowhere near the cause. You already solved this correctly two lines below for `getKeyInfo` — could you use the same shape here? ```java assertThat(getOpenKeyInfo(BUCKET_LAYOUT)) .extracting(OmKeyInfo::getKeyName) .contains(key1.getName(), key2.getName()); ``` That keeps the real assertion (both keys are open) without coupling it to unrelated cluster state. ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java: ########## @@ -518,6 +518,7 @@ public static TraceCloseable createActivatedSpan(String spanName) { return createActivatedSpan(spanName, SpanKind.INTERNAL); } + @SuppressWarnings("MustBeClosedChecker") Review Comment: Nit: the suppression is correct — the returned `TraceCloseable` lambda does `scope.close(); span.end();` — but a bare `@SuppressWarnings` records no invariant. If someone later adds an early return, or returns the `Scope` directly, the annotation silently keeps the checker quiet and you leak a thread-local scope. Either a one-line comment ("the returned TraceCloseable closes the Scope and ends the Span"), or annotating the method `@MustBeClosed` so the obligation propagates to callers instead of being suppressed. Same for the one at line 620. ########## hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDiffUtils.java: ########## @@ -106,10 +105,7 @@ validSstFile, new SstFileInfo(validSstFile, validSSTFileStartRange, validSSTFile RocksDiffUtils.filterRelevantSstFiles(inputSstFiles, tablesToLookup, new TablePrefixInfo( - new HashMap<String, String>() {{ - put(invalidColumnFamilyName, getLexicographicallyHigherString(invalidSSTFileEndRange)); - put(validSSTColumnFamilyName, expectedPrefix); - }})); + ImmutableMap.of(validSSTColumnFamilyName, expectedPrefix))); Review Comment: This drops the `invalidColumnFamilyName` entry rather than de-duplicating it. It's equivalent *today* only because every row in `values()` passes `"validColumnFamily"` for both `validSSTColumnFamilyName` and `invalidColumnFamilyName`, so the second `put` overwrote the first. The problem is that the rewrite bakes that coincidence into the test. The obvious way to strengthen this test later is to add a row where the two column families actually differ — and at that point, instead of failing loudly, `filterRelevantSstFiles` would silently run with no prefix registered for the invalid column family and exercise a different branch than the test name implies. (`ImmutableMap.of` would also throw on the duplicate key if someone restored the second entry.) Preserving the original intent while still satisfying Error Prone: ```java Map<String, String> prefixes = new HashMap<>(); prefixes.put(invalidColumnFamilyName, getLexicographicallyHigherString(invalidSSTFileEndRange)); prefixes.put(validSSTColumnFamilyName, expectedPrefix); ... new TablePrefixInfo(prefixes) ... ``` Or, if you'd rather keep the `ImmutableMap`, a one-line comment noting that the two parameters are intentionally always equal so the invalid entry is unreachable. -- 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]
