Jackie-Jiang commented on code in PR #19828:
URL: https://github.com/apache/pinot/pull/19828#discussion_r4265026201
##########
pinot-common/pom.xml:
##########
@@ -201,7 +201,7 @@
</dependency>
<dependency>
<groupId>org.apache.arrow</groupId>
- <artifactId>arrow-memory-netty</artifactId>
+ <artifactId>arrow-memory-unsafe</artifactId>
Review Comment:
The unsafe backend removes an effective memory limit from broker Arrow
responses. `ArrowResponseEncoder` constructs `RootAllocator(Long.MAX_VALUE)`,
so it relies on the backing allocator for a finite limit. Previously, ordinary
Arrow buffers went through Netty's direct allocator, which enforces the
configured direct-memory budget. [Arrow 19's
UnsafeAllocationManager](https://github.com/apache/arrow-java/blob/v19.0.0/memory/memory-unsafe/src/main/java/org/apache/arrow/memory/unsafe/UnsafeAllocationManager.java)
instead calls `MemoryUtil.allocateMemory`, which delegates directly to
`Unsafe.allocateMemory` without enforcing `MaxDirectMemorySize` or
`io.netty.maxDirectMemory`.
Large or concurrent ARROW response batches can therefore exceed the former
limit and exhaust container memory instead of failing allocation at the
configured budget. Please preserve a finite shared Arrow allocation budget, or
use a compatible backend that retains the existing cap. The existing round-trip
and per-reader allocator-limit tests do not cover this process-wide budget
change.
##########
pom.xml:
##########
@@ -218,7 +218,7 @@
<libthrift.version>0.25.0</libthrift.version>
<log4j.version>2.26.1</log4j.version>
<slf4j.version>2.0.20</slf4j.version>
- <netty.version>4.1.139.Final</netty.version>
+ <netty.version>4.2.18.Final</netty.version>
Review Comment:
The hostname-verification default change also affects the pinned ZooKeeper
3.9.6 client. `ClientX509Util.createNettySslContextForClient()` builds
`SslContextBuilder.forClient()` without clearing endpoint identification; when
`zookeeper.ssl.hostnameVerification=false`, it simply returns the built
context. [Netty 4.2 now defaults that context to HTTPS endpoint
identification](https://netty.io/wiki/netty-4.2-migration-guide.html), so the
ZooKeeper setting no longer disables verification.
For an existing TLS ZooKeeper deployment with a CA-trusted certificate whose
SAN does not match the connection hostname, Pinot will fail to establish its
ZooKeeper session after this upgrade. The override in `TlsUtils` only covers
Pinot's own transport context. Please preserve ZooKeeper's configured hostname
policy as part of this migration, and cover a trusted mismatched-host
certificate with verification disabled. This finding is based on source
inspection; I have not run a handshake reproduction.
--
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]
