HTHou commented on code in PR #17945: URL: https://github.com/apache/iotdb/pull/17945#discussion_r3449734080
########## iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingTransportWrapper.java: ########## @@ -27,8 +27,8 @@ import java.nio.channels.SocketChannel; /** - * In Thrift 0.14.1, TNonblockingSocket's constructor throws a never-happened exception. So, we - * screen the exception https://issues.apache.org/jira/browse/THRIFT-5412 + * TNonblockingSocket's constructor throws a never-happened exception. So, we screen the exception + * https://issues.apache.org/jira/browse/THRIFT-5412 */ Review Comment: Fixed in `1eba3b7d4e` by rewording the Javadoc to explain that the constructor declares `TTransportException` for compatibility, but this code path is not expected to throw it. ########## .github/workflows/client-cpp-package.yml: ########## @@ -46,18 +46,32 @@ jobs: steps: - uses: actions/checkout@v5 if: github.event_name == 'push' && startsWith(github.ref, 'refs/heads/rc/') - - uses: dorny/paths-filter@v3 + with: + fetch-depth: 0 + - name: Detect C++ package changes id: filter if: github.event_name == 'push' && startsWith(github.ref, 'refs/heads/rc/') - with: - filters: | - cpp: - - 'iotdb-client/client-cpp/**' - - 'iotdb-client/pom.xml' - - 'iotdb-protocol/thrift-datanode/src/main/thrift/client.thrift' - - 'iotdb-protocol/thrift-commons/src/main/thrift/common.thrift' - - '.github/workflows/client-cpp-package.yml' - - '.github/scripts/package-client-cpp-*.sh' + shell: bash + run: | + set -euo pipefail + + BASE="${{ github.event.before }}" + HEAD="${{ github.sha }}" + if [[ "$BASE" =~ ^0+$ ]]; then + BASE="$(git hash-object -t tree /dev/null)" + fi + + cpp=false + while IFS= read -r file; do + case "$file" in + iotdb-client/client-cpp/*|iotdb-client/pom.xml|iotdb-protocol/thrift-datanode/src/main/thrift/client.thrift|iotdb-protocol/thrift-commons/src/main/thrift/common.thrift|.github/workflows/client-cpp-package.yml|.github/scripts/package-client-cpp-*.sh) + cpp=true + break + ;; + esac + done < <(git diff --name-only "$BASE" "$HEAD") Review Comment: I checked this one and did not change the workflow. The path is matched with Bash `case` string pattern matching, where `*` matches `/`; a path like `iotdb-client/client-cpp/src/...` is matched by `iotdb-client/client-cpp/*`. ########## LICENSE-binary: ########## @@ -241,7 +241,7 @@ org.eclipse.jetty.ee10:jetty-ee10-servlet:12.0.36 org.eclipse.jetty:jetty-util:12.0.36 com.google.code.findbugs:jsr305:3.0.2 Review Comment: Addressed in `1eba3b7d4e` by excluding `org.apache.commons:commons-lang3` from the managed `libthrift` dependency. I also verified that `iotdb-core/datanode` and `iotdb-core/confignode` no longer resolve `commons-lang3` via `mvn dependency:tree -Dincludes=org.apache.commons:commons-lang3`, and confirmed IoTDB does not use `org.apache.thrift.partial` APIs. ########## .github/workflows/multi-language-client.yml: ########## @@ -50,36 +50,55 @@ jobs: go: ${{ steps.filter.outputs.go }} steps: - uses: actions/checkout@v5 - - uses: dorny/paths-filter@v3 - id: filter with: - filters: | - cpp: - - 'iotdb-client/pom.xml' - - 'iotdb-client/client-cpp/**' - - 'iotdb-protocol/thrift-datanode/src/main/thrift/client.thrift' - - 'iotdb-protocol/thrift-commons/src/main/thrift/common.thrift' - - '.github/workflows/multi-language-client.yml' - - '.github/workflows/client-cpp-package.yml' - - '.github/scripts/package-client-cpp-*.sh' - python: - - 'pom.xml' - - 'iotdb-client/pom.xml' - - 'iotdb-client/client-py/**' - - 'docker/src/main/Dockerfile-1c1d' - - '.github/workflows/multi-language-client.yml' - go: - - 'pom.xml' - - 'iotdb-core/**' - - 'iotdb-api/**' - - 'iotdb-protocol/**' - - 'distribution/**' - - 'integration-test/**' - - 'iotdb-client/session/**' - - 'iotdb-client/jdbc/**' - - 'iotdb-client/cli/**' - - 'iotdb-client/service-rpc/**' - - '.github/workflows/multi-language-client.yml' + fetch-depth: 0 + - name: Detect changed client paths + id: filter + shell: bash + run: | + set -euo pipefail + + if [[ "${{ github.event_name }}" == "pull_request" ]]; then + BASE="${{ github.event.pull_request.base.sha }}" + HEAD="${{ github.sha }}" + elif [[ "${{ github.event_name }}" == "push" ]]; then + BASE="${{ github.event.before }}" + HEAD="${{ github.sha }}" + else + echo "cpp=true" >> "$GITHUB_OUTPUT" + echo "python=true" >> "$GITHUB_OUTPUT" + echo "go=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + + if [[ "$BASE" =~ ^0+$ ]]; then + BASE="$(git hash-object -t tree /dev/null)" + fi + + cpp=false + python=false + go=false + while IFS= read -r file; do + case "$file" in + iotdb-client/pom.xml|iotdb-client/client-cpp/*|iotdb-protocol/thrift-datanode/src/main/thrift/client.thrift|iotdb-protocol/thrift-commons/src/main/thrift/common.thrift|.github/workflows/multi-language-client.yml|.github/workflows/client-cpp-package.yml|.github/scripts/package-client-cpp-*.sh) + cpp=true + ;; + esac + case "$file" in + pom.xml|iotdb-client/pom.xml|iotdb-client/client-py/*|docker/src/main/Dockerfile-1c1d|.github/workflows/multi-language-client.yml) + python=true + ;; + esac + case "$file" in + pom.xml|iotdb-core/*|iotdb-api/*|iotdb-protocol/*|distribution/*|integration-test/*|iotdb-client/session/*|iotdb-client/jdbc/*|iotdb-client/cli/*|iotdb-client/service-rpc/*|.github/workflows/multi-language-client.yml) + go=true + ;; + esac + done < <(git diff --name-only "$BASE" "$HEAD") Review Comment: I checked this one and did not change the workflow. These patterns are evaluated by Bash `case "$file" in ... )` string matching, not pathname expansion. In Bash `case`, `*` matches `/`, so `iotdb-client/client-cpp/*`, `iotdb-core/*`, and similar patterns do match nested paths such as `iotdb-client/client-cpp/src/...`. ########## iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TSocketWrapper.java: ########## @@ -24,8 +24,8 @@ import org.apache.thrift.transport.TTransportException; /** - * In Thrift 0.14.1, TSocket's constructor throws a never-happened exception. So, we screen the - * exception https://issues.apache.org/jira/browse/THRIFT-5412 + * TSocket's constructor throws a never-happened exception. So, we screen the exception + * https://issues.apache.org/jira/browse/THRIFT-5412 */ Review Comment: Fixed in `1eba3b7d4e` by rewording the Javadoc to explain that the constructor declares `TTransportException` for compatibility, but this code path is not expected to throw it. -- 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]
