You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by nsuke <gi...@git.apache.org> on 2016/09/04 09:44:18 UTC
[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...
GitHub user nsuke opened a pull request:
https://github.com/apache/thrift/pull/1081
THRIFT-3854 add a way in java to clear TFramedTransport read buffers
imported from JIRA issue.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nsuke/thrift THRIFT-3854
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1081.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1081
----
commit 839b88a4fa836bd30fa01fd047d9a4ba5b138a87
Author: Chris Lockfort <cl...@palantir.com>
Date: 2016-06-16T21:52:19Z
THRIFT-3854 add a way in java to clear TFramedTransport read buffers
The use case that I would like this for is that I have a
large number of Thrift connections in a connection pool,
and upon return the used connection to the pool I would
like to eagerly clean up the read buffer because I know
it will not be used again until the connection is checked
out and a new read puts a new buffer in its place.
This saves a considerable amount of heap memory in my application.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...
Posted by clockfort <gi...@git.apache.org>.
Github user clockfort commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1081#discussion_r98236369
--- Diff: lib/java/src/org/apache/thrift/transport/TFramedTransport.java ---
@@ -123,6 +123,12 @@ public void consumeBuffer(int len) {
readBuffer_.consumeBuffer(len);
}
+ public void clear() {
+ if (readBuffer_ != null) {
--- End diff --
(I'm the person who originally wrote this patch / feature request)
I agree, and can't find any reflected use cases in at least just the thrift codebase.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1081: THRIFT-3854 add a way in java to clear TFramedTransport ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1081
I made the requested changes locally but I don't know if I can push on top of this, so I will likely open a new pull request to obsolete this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1081#discussion_r98086075
--- Diff: lib/java/test/org/apache/thrift/transport/TestTFastFramedTransport.java ---
@@ -19,6 +19,8 @@
package org.apache.thrift.transport;
public class TestTFastFramedTransport extends TestTFramedTransport {
+ protected final static int INITIAL_CAPACITY = 50;
+
@Override
protected TTransport getTransport(TTransport underlying) {
return new TFastFramedTransport(underlying, 50, 10 * 1024 * 1024);
--- End diff --
I think 50 was supposed to be changed to ``INITIAL_CAPACITY`` here, something I could do as part of the merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1081
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...
Posted by jsirois <gi...@git.apache.org>.
Github user jsirois commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1081#discussion_r98087986
--- Diff: lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java ---
@@ -164,6 +166,10 @@ public void consumeBuffer(int len) {
readBuffer.consumeBuffer(len);
}
+ public void clear() {
+ readBuffer = new AutoExpandingBufferReadTransport(initialBufferCapacity, 1.5);
--- End diff --
It would be nice to share the initialization code on line 110 above in the constructor - consider factoring out a factory or calling reset from the constructor to initialize `readBuffer` for the 1st time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...
Posted by clockfort <gi...@git.apache.org>.
Github user clockfort commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1081#discussion_r98235950
--- Diff: lib/java/test/org/apache/thrift/transport/TestTFastFramedTransport.java ---
@@ -19,6 +19,8 @@
package org.apache.thrift.transport;
public class TestTFastFramedTransport extends TestTFramedTransport {
+ protected final static int INITIAL_CAPACITY = 50;
+
@Override
protected TTransport getTransport(TTransport underlying) {
return new TFastFramedTransport(underlying, 50, 10 * 1024 * 1024);
--- End diff --
Yes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...
Posted by jsirois <gi...@git.apache.org>.
Github user jsirois commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1081#discussion_r98087206
--- Diff: lib/java/src/org/apache/thrift/transport/TFramedTransport.java ---
@@ -123,6 +123,12 @@ public void consumeBuffer(int len) {
readBuffer_.consumeBuffer(len);
}
+ public void clear() {
+ if (readBuffer_ != null) {
--- End diff --
`readBuffer_` is declared `private TMemoryInputTransport readBuffer_ = new TMemoryInputTransport(new byte[0]);` and never re-assigned in this class. Unless there is known code re-assigning via reflection, this should just be promoted to final and all the null checks - like the one added here - dispensed with.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---