Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-35234: [Go] Fix skip argument to Callers #35231

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

hermanschaaf
Copy link
Contributor

@hermanschaaf hermanschaaf commented Apr 19, 2023

Follow-up to #35215. It's mostly cosmetic, but without the additional 2 skips passed in to Callers, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇

@github-actions
Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@hermanschaaf hermanschaaf changed the title [Go] Fix skip argument to Callers GH-35234: [Go] Fix skip argument to Callers Apr 19, 2023
@github-actions
Copy link

@github-actions
Copy link

⚠️ GitHub issue #35234 has been automatically assigned in GitHub to PR creator.

@disq
Copy link
Contributor

disq commented Apr 20, 2023

Test failure doesn't seem relevant

@zeroshade
Copy link
Member

@hermanschaaf can you try rebaseing from main and seeing if that alleviates the test failure?

@hermanschaaf
Copy link
Contributor Author

@zeroshade Just updated to the latest main; looks like all the tests passed except for Go / AMD64 Debian 11 Go 1.18. The test failure still seems unrelated to this PR:

--- FAIL: TestPreparedStatementNoSchema (0.01s)
    driver_test.go:744: 
        	Error Trace:	/arrow/go/arrow/flight/flightsql/driver/driver_test.go:744
        	Error:      	Received unexpected error:
        	            	EOF
        	Test:       	TestPreparedStatementNoSchema
FAIL
FAIL	github.com/apache/arrow/go/v12/arrow/flight/flightsql/driver	1.150s

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, the failure is unrelated to this PR. if it fails in main after merging this I'll file an issue to look into it.

@zeroshade zeroshade merged commit c48916a into apache:main Apr 21, 2023
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Apr 21, 2023
@ursabot
Copy link

ursabot commented Apr 23, 2023

Benchmark runs are scheduled for baseline = 1a697ab and contender = c48916a. c48916a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Finished ⬇️1.02% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.15% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] c48916a5 ec2-t3-xlarge-us-east-2
[Failed] c48916a5 test-mac-arm
[Finished] c48916a5 ursa-i9-9960x
[Finished] c48916a5 ursa-thinkcentre-m75q
[Finished] 1a697abd ec2-t3-xlarge-us-east-2
[Failed] 1a697abd test-mac-arm
[Finished] 1a697abd ursa-i9-9960x
[Finished] 1a697abd ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
Follow-up to apache#35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: apache#35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
Follow-up to apache#35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: apache#35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
rtpsw pushed a commit to rtpsw/arrow that referenced this pull request May 16, 2023
Follow-up to apache#35215. It's mostly cosmetic, but without the additional 2 skips passed in to `Callers`, the Caller frame is repeated in the stack trace, and one frame below it is added as well. With this change, the checked allocator stack trace contains no duplicates.

I had this change locally but didn't realize it wasn't pushed to the PR branch, sorry about that 🙇
* Closes: apache#35234

Authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@candiduslynx candiduslynx deleted the fix-allocators-skip branch June 2, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Go] Two additional frames need to be skipped in Checked Allocator output
4 participants