You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/13 20:29:53 UTC
[GitHub] [arrow] westonpace opened a new pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
westonpace opened a new pull request #10019:
URL: https://github.com/apache/arrow/pull/10019
Reverts the streaming CSV reader and the async workaround introduced for it. It will be reintroduced, more cleanly, in ARROW-12355
--
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] [arrow] github-actions[bot] commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819238662
https://issues.apache.org/jira/browse/ARROW-12161
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819212589
And for the last confirmation I modified the test to use a smaller block size and the test passes: https://github.com/westonpace/arrow/runs/2339553058?check_suite_focus=true
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819803531
Rebased. I'll watch local CI.
--
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] [arrow] lidavidm commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819874679
Ok, I don't think the Travis queue is clearing anytime soon. https://github.com/lidavidm/arrow/tree/arrow-11797 which is this + ARROW-11797 passes Actions/Travis/AppVeyor, so I'll merge this and rebase 11797.
--
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] [arrow] lidavidm edited a comment on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
lidavidm edited a comment on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819073155
I kicked AppVeyor as the Windows arrow-dataset-file-csv-test failed seemingly without explanation: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/38687464/job/pth19ssutpbagn0s
but it may very well be a Windows-only issue (the test does pass locally for me on Linux)
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819087942
@lidavidm It does indeed seem to be a Windows only issue. Local builds failed twice for me so I'm currently building on my laptop to investigate.
--
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] [arrow] lidavidm commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819796252
@westonpace would you like to rebase this and check that Windows tests pass now that we've reverted mimalloc?
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819200497
Just to be clear, ARROW-12220 is still a separate and valid bug. I'm not implying that ARROW-12220 is a fault of mimalloc. I started writing the first half of this message assuming ARROW-12220 was the fix and then probably should've rewritten or just removed that part.
--
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] [arrow] lidavidm closed pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #10019:
URL: https://github.com/apache/arrow/pull/10019
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819197009
Ok, after getting lost in the weeds for a while I was able to confirm that this is very much related to ARROW-12220. Some fun facts...
* Since threading has been removed the teardown is a lot more deterministic so we get the error 100% of the time
* It only happens on the Windows build that has mimalloc turned on
* The reason there are no logs is because the failure is not a segmentation fault but a mimalloc assertion
* The assertion happens as the background generator is being destroyed (I verified this with the debugger and this is pretty concrete evidence)
Unfortunately, merging in ARROW-12220 did not fix the issue. So...more debugging and I was able to discover...
https://github.com/microsoft/mimalloc/issues/363
It is triggered by a thread exit. The serial thread readers use a dedicated thread that is destroyed when the reader finishes. That thread must have done a huge allocation. Huge is defined as "larger than `1<<21`". The test in question that triggers this uses a block size of `1<<22`
So, I can workaround it by changing the `block_size` in the test. We could conceivably limit the block size for users. I'm not fully aware of all the places we destroy threads (there aren't many so we can maybe get away with it). We may want to reconsider mimalloc 2.0 until this is fixed.
--
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] [arrow] lidavidm commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819834271
I'll give CI some more time but I'll merge this tonight and then rebase ARROW-11797. I've speculatively rebased the latter onto this branch at: https://github.com/lidavidm/arrow/tree/arrow-11797 so we can catch anything in CI; hopefully we'll have this all merged tonight or early tomorrow.
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819197286
@nealrichardson @jonkeane Tagging the mimalloc crowd.
--
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] [arrow] lidavidm commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819073155
I kicked AppVeyor as the Windows test failed seemingly without explanation: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/38687464/job/pth19ssutpbagn0s
but it may very well be a Windows-only issue (the test does pass locally)
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819032539
CC @lidavidm Sorry, I hadn't realized you were also working on this. This revert is a bit more extensive than yours as it removes some stuff that was put in just to get the async streaming reader working.
--
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] [arrow] lidavidm commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819835139
Actually, it looks like between your fork's CI and AppVeyor here, all the relevant (C++, Python, R, etc.) CI jobs look good to go, with only Travis being queued.
--
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] [arrow] westonpace commented on pull request #10019: ARROW-12161: [C++][Dataset] Revert async CSV reader in datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10019:
URL: https://github.com/apache/arrow/pull/10019#issuecomment-819822419
Appveyor & local Windows 2019 builds pass so that is promising.
--
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