[
https://issues.apache.org/jira/browse/CALCITE-3325?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16956310#comment-16956310
]
Josh Elser commented on CALCITE-3325:
-------------------------------------
{quote}Finally `UnsynchronizedBuffer.reset()` is invoked to make sure the
UnsynchronizedBuffer is ready for next invocation (which never happens because
on next invocation a new instance will be created :( ).
{quote}
I think this is the source of my confusion. Server-side, we generally have a
pool of threads which work incoming client requests. This is bounded to ensure
that the server won't be overrun if there's an influx of clients. If, within a
single thread, we're not getting the same instance of an UnsynchronizedBuffer,
then this code doesn't work right.
The same _should_ apply client side, but there's more ability for you to shoot
yourself in the foot (if you run a single operation on a thread and throw that
thread away). Because you're creating a new Connection every time, you're
throwing away any state we had for that thread, and re-creating everything. Why
aren't you keeping that Connection around?
The ThreadLocal is intended to be used as a "poor-man's" pool. I intended the
ThreadLocal to be a way that scales linearly with what the client is doing –
not that you, as the developer, have to be aware of some arcane configuration
property that you have to scale up as you vertically scale up your Avatica
client code (if that makes sense).
I appreciate you sharing the client code. I'll have to try to find some time
this week to make that functional (I don't have an avatica test harness ready
to go right now), and can look at that. If you have the cycles, I'd be curious
what your test shows if you changed it to do..
{code:java}
public void run() {
try (Connection con =
DriverManager.getConnection("jdbc:phoenix:thin:url=" + URL) {
while (true) {
try (PreparedStatement psmt =
con.prepareStatement("SELECT 1")) {
ResultSet rs = psmt.executeQuery();
rs.next();
Thread.sleep(100);
}
}
} catch (Exception e) { throw new RuntimeException(e); }
}
{code}
> Thread Local Buffer Variable (threadLocalBuffer) In ProtobufTranslationImpl
> Is Defined As Non Static Causing Memory Leak
> ------------------------------------------------------------------------------------------------------------------------
>
> Key: CALCITE-3325
> URL: https://issues.apache.org/jira/browse/CALCITE-3325
> Project: Calcite
> Issue Type: Bug
> Reporter: Mehdi Salarkia
> Priority: Major
> Attachments: Non-Static.snapshot, Non-static.png, Screen Shot
> 2019-09-05 at 5.05.20 PM.png, Screen Shot 2019-09-05 at 5.18.19 PM.png,
> Static.png, Static.snapshot
>
>
> As we were load testing our system on Apache Phoenix via the thin client
> which uses Avatica we ran into Garbage collection problems. After some
> investigation we could see there are a lot of unreferenced object due to this
> variable:
> {code:java}
> private final ThreadLocal<UnsynchronizedBuffer> threadLocalBuffer =
> new ThreadLocal<UnsynchronizedBuffer>() {
> @Override protected UnsynchronizedBuffer initialValue() {
> return new UnsynchronizedBuffer();
> }
> };
> {code}
> Which seems to be a reusable buffer for serializing/deserializing the data.
> From my understating there is a copy of this variable created per each
> instance of ProtobufTranslationImpl. However the proper use of ThreadLocal it
> seems to be one instance per thread and the current implementation seems to
> be missing the `static` keyword when defining the thread local variable:
> See [https://docs.oracle.com/javase/7/docs/api/java/lang/ThreadLocal.html]
> {code:java}
> ThreadLocal instances are typically private static fields in classes that
> wish to associate state with a thread (e.g., a user ID or Transaction ID).
> {code}
> See the attached snapshot from a memory dump we took.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
