You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2021/03/26 00:09:56 UTC
[GitHub] [thrift] fishy opened a new pull request #2359: THRIFT-5369: Add MaxContainerSize to go's TConfiguration
fishy opened a new pull request #2359:
URL: https://github.com/apache/thrift/pull/2359
Client: go
And check for that in TProtocols' Read{Map|List|Set}Begin
implementations.
Also remove size arg from make() calls in compiler.
<!-- Explain the changes in the pull request below: -->
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] jpkrohling commented on pull request #2359: THRIFT-5369: Use MaxMessageSize to check container sizes
Posted by GitBox <gi...@apache.org>.
jpkrohling commented on pull request #2359:
URL: https://github.com/apache/thrift/pull/2359#issuecomment-814847385
I still wasn't able to test this, but a Jaeger community member (@forestsword) was able to test it and confirmed that this indeed fixes the memory leak.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2359: THRIFT-5369: Use MaxMessageSize to check container sizes
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2359:
URL: https://github.com/apache/thrift/pull/2359#issuecomment-808492770
@jpkrohling can you try this version to see if you can still reproduce the bug? (note that you need to set a smaller, sane MaxMessageSize to TConfiguration to actual limit it. the default limit is 100M)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on pull request #2359: THRIFT-5369: Add MaxContainerSize to go's TConfiguration
Posted by GitBox <gi...@apache.org>.
dcelasun commented on pull request #2359:
URL: https://github.com/apache/thrift/pull/2359#issuecomment-808283377
Seems fine from a quick a look, but it'd be great if someone from jaeger can verify it as well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2359: THRIFT-5369: Use MaxMessageSize to check container sizes
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2359:
URL: https://github.com/apache/thrift/pull/2359#issuecomment-808490743
@Jens-G I updated this PR to remove MaxContainerSize from TConfiguration and use MaxMessageSize to check for container sizes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2359: THRIFT-5369: Use MaxMessageSize to check container sizes
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2359:
URL: https://github.com/apache/thrift/pull/2359#issuecomment-815943220
As mentioned in https://github.com/jaegertracing/jaeger/issues/2638#issuecomment-815859394, I reverted the compiler changes from this PR as they have negative performance impacts for everyone and it doesn't seem they are really necessary to fix the memory issue. We can have a follow up PR to add them back if they are proven to be necessary.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2359: THRIFT-5369: Use MaxMessageSize to check container sizes
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2359:
URL: https://github.com/apache/thrift/pull/2359#discussion_r602608875
##########
File path: CHANGES.md
##########
@@ -1,5 +1,17 @@
# Apache Thrift Changelog
+## 0.15.0
+
+### Deprecated Languages
+
+### Removed Languages
+
+### Breaking Changes
+
+### Go
+
+- [THRIFT-5369](https://issues.apache.org/jira/browse/THRIFT-5369) - No longer pre-allocation the whole container (map/set/list) in compiled go code to avoid huge allocations on malformed messages.
Review comment:
nit
```suggestion
- [THRIFT-5369](https://issues.apache.org/jira/browse/THRIFT-5369) - No longer pre-allocating the whole container (map/set/list) in compiled go code to avoid huge allocations on malformed messages.
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] jpkrohling commented on pull request #2359: THRIFT-5369: Use MaxMessageSize to check container sizes
Posted by GitBox <gi...@apache.org>.
jpkrohling commented on pull request #2359:
URL: https://github.com/apache/thrift/pull/2359#issuecomment-810543038
@fishy sorry for being not very responsive here. I'm on PTO until next Tuesday, so I probably can't test this before then but I am interested in checking whether this solves our problem!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy merged pull request #2359: THRIFT-5369: Use MaxMessageSize to check container sizes
Posted by GitBox <gi...@apache.org>.
fishy merged pull request #2359:
URL: https://github.com/apache/thrift/pull/2359
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org