You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "onichols-pivotal (GitHub)" <gi...@apache.org> on 2019/01/25 21:30:06 UTC
[GitHub] [geode] onichols-pivotal opened pull request #3124:
GEODE-6310 upgrade classgraph version to fix leaking file descriptors
* bumps classgraph from 4.0.6 => 4.4.12
* NOTE: raises the smallest viable heap size to start a locator
from -Xmx40m to -Xmx65m
* removes usage of a non-public classgraph API
* properly wraps all scanner uses in try-with-resources to ensure
temporary files are cleaned up and file descriptors are closed
* auto-detects whether to optimize for speed or memory footprint,
based on total heap size. Less than 256m (common in testing or
single-laptop demo environments) will use single-threaded scanner,
while production-sized locators will take full advantage of
classgraph's highly-tuned multi-threaded scanning capability.
* please note that classgraph 4.6.18 was not selected because 4.6.x
has breaking changes that will require more extensive porting.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes? _Existing tests are sufficient._
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
[ Full content available at: https://github.com/apache/geode/pull/3124 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #3124:
GEODE-6310 limit classgraph file descriptor consumption
Posted by "onichols-pivotal (GitHub)" <gi...@apache.org>.
fair enough @jdeppe-pivotal, it's "magic code like this" is classgraph that prompted this change in the first place. I would even go a step beyond @metatype's suggestion and just lock it to "1"
[ Full content available at: https://github.com/apache/geode/pull/3124 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #3124:
GEODE-6310 limit classgraph file descriptor consumption
Posted by "onichols-pivotal (GitHub)" <gi...@apache.org>.
changed to always use 1 thread
[ Full content available at: https://github.com/apache/geode/pull/3124 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal closed pull request #3124: GEODE-6310
limit classgraph file descriptor consumption
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
[ pull request closed by jdeppe-pivotal ]
[ Full content available at: https://github.com/apache/geode/pull/3124 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] metatype commented on pull request #3124: GEODE-6310
limit classgraph file descriptor consumption
Posted by "metatype (GitHub)" <gi...@apache.org>.
Would setting it to "2" simplify the logic and runtime expectations?
[ Full content available at: https://github.com/apache/geode/pull/3124 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #3124:
GEODE-6310 limit classgraph file descriptor consumption
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
I really don't like 'magic' code like this. In general, heuristics like this take some testing to get right and typically we'll never cover every eventuality. That means they need to allow overriding by setting some kind of system property which is also not ideal. In this particular case (and from the testing I've done with Classgraph) the number of threads has no correlation either with memory consumption or open file descriptors. Other than this bit, the rest LGTM.
[ Full content available at: https://github.com/apache/geode/pull/3124 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #3124:
GEODE-6310 limit classgraph file descriptor consumption
Posted by "onichols-pivotal (GitHub)" <gi...@apache.org>.
fair enough @jdeppe-pivotal, it's "magic code like this" in classgraph that prompted this change in the first place. I would even go a step beyond @metatype's suggestion and just lock it to "1"
[ Full content available at: https://github.com/apache/geode/pull/3124 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org