Skip to content

Bump Thrift to 0.25.0 - #18793

Open
HTHou wants to merge 8 commits into
apache:masterfrom
HTHou:codex/upgrade-thrift-0.25
Open

HTHou wants to merge 8 commits into
apache:masterfrom
HTHou:codex/upgrade-thrift-0.25

Conversation

@HTHou

@HTHou HTHou commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Description

  • Upgrade Apache Thrift Java runtime from 0.24.0 to 0.25.0.
  • Upgrade iotdb-tools-thrift from 0.23.0.0 to 0.25.0.0.
  • Add the Apache staging repository orgapacheiotdb-1203 so CI can resolve the release candidate.
  • Update LICENSE-binary accordingly.

Tests

  • GitHub Actions CI.

@HTHou
HTHou requested a balanced review from Copilot October 6, 2026 13:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Frame-bound allocation checks remain ineffective, and the CI matrix adds a deprecated runner.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Upgrades the Java Thrift runtime and compiler tooling to 0.25.0 while adapting custom transports and CI.

Changes:

  • Bumps Thrift dependencies and licensing metadata.
  • Adds message-budget reset support and regression coverage.
  • Expands multi-platform client CI.
File Description
pom.xml Updates Thrift versions and adds staging repository.
LICENSE-binary Updates bundled libthrift version.
TElasticFramedTransport.java Resets frame message-size budgets.
NonOpenTransport.java Implements the new transport API method.
TElasticFramedTransportTest.java Tests consecutive frame reads.
multi-language-client.yml Adds ARM CI and architecture-specific caching.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/multi-language-client.yml
@HTHou
HTHou force-pushed the codex/upgrade-thrift-0.25 branch from 44ef1a6 to ee8852d Compare October 6, 2026 14:14
@HTHou
HTHou requested a balanced review from Copilot October 6, 2026 14:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The compressed/Snappy transport bypasses the new per-frame budget reset and can fail on long-lived connections.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)

@HTHou
HTHou requested a balanced review from Copilot October 6, 2026 14:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Root-POM-only updates still skip the C++ job because the internal path detector omits pom.xml.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Include pom.xml in C++ changed-path detection

.github/​workflows/​multi-language-client.yml:9

Adding pom.xml only to the workflow-level path filter does not enable C++ validation for a root-POM-only change: the cpp case in Detect changed client paths still omits pom.xml, so updates such as iotdb-tools-thrift.version start this workflow but leave the C++ job skipped. Add pom.xml to that case as well.

@HTHou
HTHou requested a balanced review from Copilot October 6, 2026 15:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Message-budget limits can reject valid large frames, and the new tests do not reliably detect missing resets.

Review effort: Balanced
Findings: 2 Medium severity · 1 Low severity

Open (3)
Resolved since last review (1)

validateFrame(size);
readBuffer.fill(underlying, size);
// Bind subsequent protocol reads to the current compressed frame size.
resetMessageSizeAndConsumedBytes(size);
validateFrame(size);
readBuffer.fill(underlying, size);
// Bind subsequent protocol reads to the current frame size.
resetMessageSizeAndConsumedBytes(size);
.flip();
TConfiguration configuration =
TConfiguration.custom().setMaxMessageSize(10).setMaxFrameSize(10).build();
TMemoryBuffer underlying = new TMemoryBuffer(configuration, 10);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants