You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "HTHou (via GitHub)" <gi...@apache.org> on 2026/10/06 10:33:01 UTC
[PR] Bump Thrift to 0.25.0 [iotdb]
HTHou opened a new pull request, #18793:
URL: https://github.com/apache/iotdb/pull/18793
## 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.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bump Thrift to 0.25.0 [iotdb]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou merged PR #18793:
URL: https://github.com/apache/iotdb/pull/18793
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bump Thrift to 0.25.0 [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18793:
URL: https://github.com/apache/iotdb/pull/18793#discussion_r4197134675
##########
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TCompressedElasticFramedTransport.java:
##########
@@ -59,10 +59,14 @@ protected void closeAllocatedBuffers() {
@Override
protected void readFrame() throws TTransportException {
+ // Discard the previous frame's budget before reading the next frame header and payload.
+ resetMessageSizeAndConsumedBytes();
underlying.readAll(i32buf, 0, 4);
int size = TFramedTransport.decodeFrameSize(i32buf);
validateFrame(size);
readBuffer.fill(underlying, size);
+ // Bind subsequent protocol reads to the current compressed frame size.
+ resetMessageSizeAndConsumedBytes(size);
Review Comment:
The compressed path has the same limit mismatch: this reset validates the compressed frame size against the underlying endpoint's default `maxMessageSize`, even when this transport was constructed with a larger `thriftMaxFrameSize`. SSL/blocking endpoints using Thrift's 100 MiB default will reject otherwise-valid larger compressed frames here. Align the underlying endpoint configuration with IoTDB's configured frame limit before wrapping it.
##########
iotdb-client/service-rpc/src/test/java/org/apache/iotdb/rpc/TElasticFramedTransportTest.java:
##########
@@ -19,18 +19,85 @@
package org.apache.iotdb.rpc;
+import org.apache.thrift.TConfiguration;
import org.apache.thrift.transport.TByteBuffer;
+import org.apache.thrift.transport.TMemoryBuffer;
import org.apache.thrift.transport.TTransportException;
import org.junit.Test;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
public class TElasticFramedTransportTest {
+ @Test
+ public void testReadFramesResetMessageSize() throws TTransportException {
+ byte[] firstFrame = {1, 2};
+ byte[] secondFrame = {3, 4, 5, 6, 7, 8};
+ ByteBuffer framedData =
+ ByteBuffer.allocate(8 + firstFrame.length + secondFrame.length)
+ .putInt(firstFrame.length)
+ .put(firstFrame)
+ .putInt(secondFrame.length)
+ .put(secondFrame)
+ .flip();
+ TConfiguration configuration =
+ TConfiguration.custom().setMaxMessageSize(10).setMaxFrameSize(10).build();
+ TMemoryBuffer underlying = new TMemoryBuffer(configuration, 10);
Review Comment:
This regression test does not prove that frame budgets are reset: in Thrift 0.25, `TMemoryBuffer.read` checks each requested length but does not decrement the remaining message budget. If both new reset calls are removed, every individual read here is still at most 10 bytes and the test passes. Use a budget-consuming underlying transport so the test fails against the pre-fix implementation.
This issue also appears on line 87 of the same file.
##########
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TElasticFramedTransport.java:
##########
@@ -194,10 +194,14 @@ public int read(byte[] buf, int off, int len) throws TTransportException {
}
protected void readFrame() throws TTransportException {
+ // Discard the previous frame's budget before reading the next frame header and payload.
+ resetMessageSizeAndConsumedBytes();
underlying.readAll(i32buf, 0, 4);
int size = TFramedTransport.decodeFrameSize(i32buf);
validateFrame(size);
readBuffer.fill(underlying, size);
+ // Bind subsequent protocol reads to the current frame size.
+ resetMessageSizeAndConsumedBytes(size);
Review Comment:
`resetMessageSizeAndConsumedBytes(size)` rejects sizes above the underlying transport's `maxMessageSize`, but this wrapper's `thriftMaxFrameSize` is configured independently. Blocking server sockets and SSL client sockets are created with Thrift's 100 MiB default, while IoTDB clients may permit frames up to 512 MiB, so a valid frame between those limits is read and then fails here with `MESSAGE_SIZE_LIMIT`. Configure the endpoint message budget consistently (including the 4-byte header) before wrapping it, or otherwise reconcile the two limits before resetting.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bump Thrift to 0.25.0 [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18793:
URL: https://github.com/apache/iotdb/pull/18793#discussion_r4196462701
##########
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TElasticFramedTransport.java:
##########
@@ -194,10 +194,14 @@ public int read(byte[] buf, int off, int len) throws TTransportException {
}
protected void readFrame() throws TTransportException {
+ // Discard the previous frame's budget before reading the next frame header and payload.
+ resetMessageSizeAndConsumedBytes();
underlying.readAll(i32buf, 0, 4);
int size = TFramedTransport.decodeFrameSize(i32buf);
validateFrame(size);
readBuffer.fill(underlying, size);
+ // Bind subsequent protocol reads to the current frame size.
+ resetMessageSizeAndConsumedBytes(size);
Review Comment:
The new budget reset only runs for uncompressed frames. `TCompressedElasticFramedTransport` overrides `readFrame()` and never calls this implementation, so Snappy connections continue charging every frame against the underlying transport's single message budget and will eventually fail after enough data has been read on a persistent connection. Apply the same reset-before-header/reset-after-payload lifecycle in the compressed override (and cover the Snappy path in the regression test), or refactor the shared framing logic so overrides cannot skip 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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bump Thrift to 0.25.0 [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18793:
URL: https://github.com/apache/iotdb/pull/18793#discussion_r4205420180
##########
iotdb-client/service-rpc/src/main/i18n/en/org/apache/iotdb/rpc/i18n/RpcMessages.java:
##########
@@ -33,6 +33,8 @@ public final class RpcMessages {
"Read a negative frame size (%d)%s!";
public static final String FRAME_ERROR_FRAME_SIZE_EXCEEDED =
"Frame size (%d) larger than protect max size (%d)%s!";
+ public static final String FRAME_ERROR_FRAME_SIZE_EXCEEDS_MAX_MESSAGE_SIZE =
+ "Frame size (%d) exceeds the maximum supported message size!";
Review Comment:
This new exception-message key does not follow the repository's deterministic i18n naming convention. Because the string is passed to `TTransportException`, rename it to `EXCEPTION_FRAME_SIZE_ARG_EXCEEDS_THE_MAXIMUM_SUPPORTED_MESSAGE_SIZE_E83C0952` and update the matching zh key and call site so both locale sources remain in parity.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bump Thrift to 0.25.0 [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18793:
URL: https://github.com/apache/iotdb/pull/18793#discussion_r4196241958
##########
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TElasticFramedTransport.java:
##########
@@ -322,6 +326,11 @@ public void checkReadBytesAvailable(long numBytes) throws TTransportException {
}
}
+ @Override
+ public void resetMessageSizeAndConsumedBytes(long newSize) throws TTransportException {
+ underlying.resetMessageSizeAndConsumedBytes(newSize);
Review Comment:
The new frame-sized reset does not constrain protocol allocations. `TBinaryProtocol` calls this outer transport's `checkReadBytesAvailable`, but that override only compares against `thriftMaxFrameSize` and never consults the underlying budget reset here. A small frame can therefore still declare a very large string/binary value below the global maximum, causing that array to be allocated before the read fails. Delegate the availability check to the reset underlying transport (or validate against the current read-buffer remainder), and add a malformed-frame regression test like Thrift 0.25's frame-bound-budget test.
##########
.github/workflows/multi-language-client.yml:
##########
@@ -108,7 +110,7 @@ jobs:
fail-fast: false
max-parallel: 15
matrix:
- os: [ubuntu-22.04, ubuntu-24.04, windows-2022, windows-2025-vs2026, macos-latest]
+ os: [ubuntu-22.04, ubuntu-24.04, ubuntu-22.04-arm, windows-2022, windows-2025-vs2026, macos-latest]
Review Comment:
`ubuntu-22.04-arm` entered GitHub-hosted runner deprecation on September 17, 2026 and is now subject to intentional brownouts before retirement. Adding it to this required matrix makes client CI intermittently fail for infrastructure reasons; use the supported `ubuntu-24.04-arm` image instead.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bump Thrift to 0.25.0 [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18793:
URL: https://github.com/apache/iotdb/pull/18793#discussion_r4203819956
##########
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TElasticFramedTransport.java:
##########
@@ -100,6 +101,25 @@ public TElasticFramedTransport(
}
}
+ private void alignUnderlyingConfiguration() throws TTransportException {
+ TConfiguration configuration = underlying.getConfiguration();
+ if (configuration == null) {
+ return;
+ }
+ long maxMessageSize = (long) thriftMaxFrameSize + Integer.BYTES;
+ if (maxMessageSize > Integer.MAX_VALUE) {
+ throw new TTransportException(
+ TTransportException.MESSAGE_SIZE_LIMIT,
+ "Frame size exceeds the maximum supported message size: " + thriftMaxFrameSize);
Review Comment:
This new exception text bypasses the service-rpc compile-time localization mechanism, so the `with-zh-locale` build will still emit English. Add a matching exception constant to both the English and Chinese `RpcMessages` classes and reference that constant here.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org