You are viewing a plain text version of this content. The canonical link for it is here.
Posted to devnull@infra.apache.org by "lbartnik (GitHub)" <gi...@apache.org> on 2018/08/23 02:23:09 UTC
[GitHub] [arrow] lbartnik opened pull request #2466: Fix ARROW-1380
ARROW-1380: Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #2466: ARROW-1380:
[Plasma] Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "pitrou (GitHub)" <gi...@apache.org>.
Can this be conditioned on PLASMA_VALGRIND? We don't want to make this test suite slower than this is in the nominal case, IMHO.
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] wesm closed pull request #2466: ARROW-1380: [Plasma]
Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "wesm (GitHub)" <gi...@apache.org>.
[ pull request closed by wesm ]
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] pcmoritz commented on pull request #2466:
ARROW-1380: Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "pcmoritz (GitHub)" <gi...@apache.org>.
Can you describe why this is needed?
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] kou commented on issue #2466: Fix ARROW-1380
Posted by "kou (GitHub)" <gi...@apache.org>.
Can you use "ARROW-1380: Summary..." format for pull request title?
See also: https://cwiki.apache.org/confluence/display/ARROW/Contributing+to+Apache+Arrow
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] codecov-io commented on issue #2466: ARROW-1380: Fix
"still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=h1) Report
> Merging [#2466](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=desc) into [master](https://codecov.io/gh/apache/arrow/commit/23045d2b5be37df5cd990f5932576eb701e14b18?src=pr&el=desc) will **increase** coverage by `1.29%`.
> The diff coverage is `92.85%`.
[](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2466 +/- ##
==========================================
+ Coverage 86.14% 87.44% +1.29%
==========================================
Files 306 244 -62
Lines 46691 43028 -3663
==========================================
- Hits 40224 37624 -2600
+ Misses 6393 5404 -989
+ Partials 74 0 -74
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [cpp/src/plasma/events.h](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvZXZlbnRzLmg=) | `100% <ø> (ø)` | :arrow_up: |
| [cpp/src/plasma/events.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvZXZlbnRzLmNj) | `87.5% <100%> (-0.6%)` | :arrow_down: |
| [python/pyarrow/tests/test\_plasma.py](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-cHl0aG9uL3B5YXJyb3cvdGVzdHMvdGVzdF9wbGFzbWEucHk=) | `96.31% <100%> (+0.01%)` | :arrow_up: |
| [cpp/src/plasma/store.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvc3RvcmUuY2M=) | `90.54% <100%> (+0.9%)` | :arrow_up: |
| [cpp/src/plasma/malloc.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvbWFsbG9jLmNj) | `68.05% <75%> (+0.4%)` | :arrow_up: |
| [go/arrow/array/bufferbuilder\_byte.go](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Z28vYXJyb3cvYXJyYXkvYnVmZmVyYnVpbGRlcl9ieXRlLmdv) | | |
| [go/arrow/datatype\_nested.go](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Z28vYXJyb3cvZGF0YXR5cGVfbmVzdGVkLmdv) | | |
| [go/arrow/math/float64\_sse4\_amd64.go](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Z28vYXJyb3cvbWF0aC9mbG9hdDY0X3NzZTRfYW1kNjQuZ28=) | | |
| [rust/src/util/test\_util.rs](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-cnVzdC9zcmMvdXRpbC90ZXN0X3V0aWwucnM=) | | |
| [go/arrow/array/list.go](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Z28vYXJyb3cvYXJyYXkvbGlzdC5nbw==) | | |
| ... and [58 more](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=footer). Last update [23045d2...be8cc6c](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] lbartnik commented on pull request #2466:
ARROW-1380: Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "lbartnik (GitHub)" <gi...@apache.org>.
Sure. `file` needs to be closed, otherwise it is a memory leak. Closing it closes the descriptor `fd` but the descriptor is what this function is supposed to return. So we need a copy, close `file` and thus `fd`, and return the copy.
The real question is: why do we even need `file`? But there's a check based on that stream so I assume there is a good reason.
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] lbartnik commented on pull request #2466:
ARROW-1380: [Plasma] Fix "still reachable" valgrind warnings when
PLASMA_VALGRIND=1
Posted by "lbartnik (GitHub)" <gi...@apache.org>.
IMHO we could remove the call to `fopen` and the related check entirely. The resulting `file` is never used for reading/writing. It seems that replacing `fopen` with `open`/`_open` will give us a duplicate descriptor, and thus we will still have to close one of them: the new one of `fd`. If that's OK, before I replace `fopen` with `open`, could you please tell me why is it needed there in the first place?
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #2466: ARROW-1380:
[Plasma] Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "pitrou (GitHub)" <gi...@apache.org>.
Yeah, I don't think the `fopen` call is useful. I think you can just call `open` on Unix, and `_open` on Windows (see https://docs.microsoft.com/fr-fr/cpp/c-runtime-library/reference/open-wopen for the latter).
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] lbartnik commented on pull request #2466:
ARROW-1380: [Plasma] Fix "still reachable" valgrind warnings when
PLASMA_VALGRIND=1
Posted by "lbartnik (GitHub)" <gi...@apache.org>.
Done.
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] wesm commented on issue #2466: ARROW-1380: [Plasma]
Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "wesm (GitHub)" <gi...@apache.org>.
+1. I am going to rebase and merge this now. Thanks @lbartnik!
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [arrow] codecov-io commented on issue #2466: ARROW-1380:
[Plasma] Fix "still reachable" valgrind warnings when PLASMA_VALGRIND=1
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=h1) Report
> Merging [#2466](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=desc) into [master](https://codecov.io/gh/apache/arrow/commit/23045d2b5be37df5cd990f5932576eb701e14b18?src=pr&el=desc) will **increase** coverage by `1.3%`.
> The diff coverage is `81.81%`.
[](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2466 +/- ##
=========================================
+ Coverage 86.14% 87.45% +1.3%
=========================================
Files 306 246 -60
Lines 46691 43427 -3264
=========================================
- Hits 40224 37978 -2246
+ Misses 6393 5449 -944
+ Partials 74 0 -74
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [cpp/src/plasma/events.h](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvZXZlbnRzLmg=) | `100% <ø> (ø)` | :arrow_up: |
| [cpp/src/plasma/events.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvZXZlbnRzLmNj) | `87.5% <100%> (-0.6%)` | :arrow_down: |
| [python/pyarrow/tests/test\_plasma.py](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-cHl0aG9uL3B5YXJyb3cvdGVzdHMvdGVzdF9wbGFzbWEucHk=) | `96.61% <100%> (+0.31%)` | :arrow_up: |
| [cpp/src/plasma/store.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvc3RvcmUuY2M=) | `90.04% <66.66%> (+0.4%)` | :arrow_up: |
| [cpp/src/plasma/malloc.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvbWFsbG9jLmNj) | `68.05% <75%> (+0.4%)` | :arrow_up: |
| [cpp/src/arrow/util/thread-pool.h](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9hcnJvdy91dGlsL3RocmVhZC1wb29sLmg=) | `85.71% <0%> (-6.6%)` | :arrow_down: |
| [cpp/src/plasma/protocol.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvcHJvdG9jb2wuY2M=) | `95.36% <0%> (-3.31%)` | :arrow_down: |
| [cpp/src/arrow/adapters/orc/adapter.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9hcnJvdy9hZGFwdGVycy9vcmMvYWRhcHRlci5jYw==) | `72.98% <0%> (-2.5%)` | :arrow_down: |
| [cpp/src/arrow/test-util.h](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9hcnJvdy90ZXN0LXV0aWwuaA==) | `76.74% <0%> (-1.56%)` | :arrow_down: |
| [cpp/src/plasma/common.cc](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree#diff-Y3BwL3NyYy9wbGFzbWEvY29tbW9uLmNj) | `93.47% <0%> (-0.97%)` | :arrow_down: |
| ... and [82 more](https://codecov.io/gh/apache/arrow/pull/2466/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=footer). Last update [23045d2...10672c1](https://codecov.io/gh/apache/arrow/pull/2466?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[ Full content available at: https://github.com/apache/arrow/pull/2466 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org