ivandika3 commented on code in PR #11056:
URL: https://github.com/apache/ozone/pull/11056#discussion_r3920919744
##########
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.getLocationList()
- .stream().map(omKeyLocationInfo -> pipelines.add(
- omKeyLocationInfo.getPipeline())));
+ omKeyInfo.getKeyLocationVersions().forEach(omKeyLocationInfoGroup ->
Review Comment:
Added the non-empty pipeline regression test and updated the PR description
under “Behavior fixes included” to explicitly call out the changed
`/api/v1/containers` payload.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java:
##########
@@ -779,7 +779,7 @@ public OpenKeySession openKey(OmKeyArgs args) throws
IOException {
private OMResponse handleError(OMResponse resp) throws OMException {
if (resp.getStatus() != OK) {
throw new OMException(resp.getMessage(),
- ResultCodes.values()[resp.getStatus().ordinal()]);
+ ResultCodes.valueOf(resp.getStatus().name()));
Review Comment:
Verified this is behavior-preserving for the current enums:
`TestResultCodes` asserts equal sizes, name parity, and `valueOf(name())`
conversion for every value. I also clarified the removal of ordinal coupling in
the PR description.
##########
hadoop-ozone/dev-support/checks/errorprone.sh:
##########
@@ -0,0 +1,57 @@
+#!/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"}
+mkdir -p "$REPORT_DIR"
+
+REPORT_FILE="$REPORT_DIR/summary.txt"
+
+MAVEN_OPTIONS='-B -fae --no-transfer-progress -Perrorprone -DskipDocs
-DskipRecon -DskipShade'
+
+declare -i rc
+
+#shellcheck disable=SC2086
+mvn $MAVEN_OPTIONS test-compile "$@" | tee "${REPORT_DIR}/output.log"
+rc=$?
+
+grep -E "^\[(ERROR|WARNING)\] .*:\[[0-9]+,[0-9]+\] \[[A-Za-z][A-Za-z0-9]+\]" \
+ "${REPORT_DIR}/output.log" | awk '!seen[$0]++' > "$REPORT_FILE"
Review Comment:
The forked compiler is configured to use `en_US` through
`-J-Duser.language=en` and `-J-Duser.country=US`. The script now recognizes
both Maven (`[ERROR]`/`[WARNING]`) and raw javac (`error:`/`warning:`)
diagnostic formats; Bats fixtures cover both forms.
##########
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:
Addressed by retaining all parsed diagnostics in `diagnostics.txt`,
filtering only ERROR diagnostics to `summary.txt`, and sourcing
`_post_process.sh`. The Bats coverage now includes a Maven failure with no
Error Prone diagnostic and asserts the explicit unknown-failure summary.
##########
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:
Added `testPruneNodesRemovesNodesFromCompactionMap`. It creates an
input/output compaction pair, prunes the input node, and asserts the input is
gone from `compactionNodeMap` while the output remains.
##########
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:
Added `TestSCMSecurityExceptionErrorCodes`, which verifies all status names
map to error-code names except the two explicit historical exceptions. The
ternary now documents the legacy `GET_ROOT_CA_CERT_FAILED` naming difference,
and the test asserts the revoked-certificate status remains intentionally
unmapped.
##########
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:
Replaced both open-file-table size assertions with key-name containment
assertions. This preserves the intended verification without coupling either
test to keys left open by other methods sharing the MiniOzoneCluster.
##########
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:
Updated the PR description under “Behavior fixes included” to explicitly
disclose that Recon `/api/v1/containers` now returns populated pipeline arrays
and why the field was previously empty.
--
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]
