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

ARROW-17385: [Integration] Re-enable Rust integration case #13852

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

lidavidm
Copy link
Member

@lidavidm lidavidm commented Aug 11, 2022

@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 JIRA? https://issues.apache.org/jira/browse/ARROW

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

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

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

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

See also:

@lidavidm lidavidm changed the title DO NOT MERGE: [Integration] Re-enable Rust integration case ARROW-17385: [Integration] Re-enable Rust integration case Aug 11, 2022
@github-actions
Copy link

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @lidavidm -- I looked at the integration run https://github.com/apache/arrow/runs/7785878022?check_suite_focus=true and it definitely passes (though I didn't see anything explicitly in the logs about having run the scenario!)

@alamb
Copy link
Contributor

alamb commented Aug 11, 2022

Is there any reason to hold off merging this PR @lidavidm ?

I ask because of the somewhat scary sounding commit message DO NOT MERGE: [Integration] Re-enable Rust integration case 😅

@lidavidm
Copy link
Member Author

Thanks @lidavidm -- I looked at the integration run https://github.com/apache/arrow/runs/7785878022?check_suite_focus=true and it definitely passes (though I didn't see anything explicitly in the logs about having run the scenario!)

Look for "C++ serving, Rust requesting" then scroll down to see "Testing file middleware" - on master it says -- Skipping test because consumer Rust does not support but it runs the test here

I ask because of the somewhat scary sounding commit message DO NOT MERGE: [Integration] Re-enable Rust integration case sweat_smile

Should be good, I just put that initially in case it did fail 😅

@alamb
Copy link
Contributor

alamb commented Aug 11, 2022

==========================================================
Testing file middleware

🚀 thanks @lidavidm and @carols10cents !

@alamb alamb merged commit e2efc87 into apache:master Aug 11, 2022
@lidavidm lidavidm deleted the arrow-10961 branch August 11, 2022 15:45
@lidavidm
Copy link
Member Author

…huh, the new merge script uses the original commit and NOT the PR title like I would have expected.

@nealrichardson
Copy link
Member

nealrichardson commented Aug 11, 2022

…huh, the new merge script uses the original commit and NOT the PR title like I would have expected.

If there's only one commit in the PR, the GitHub PR merge API will use the commit message and not the PR title (by default).

@lidavidm
Copy link
Member Author

That's a little unfortunate :( I'll have to be more careful about commit messages

@nealrichardson
Copy link
Member

nealrichardson commented Aug 11, 2022

We can probably fix this going forward in the merge script, though this specific commit message should get amended.

@lidavidm
Copy link
Member Author

Hmm, is this worth force-pushing? I guess it hasn't been very long

@lidavidm
Copy link
Member Author

Or we can revert and push again

@lidavidm
Copy link
Member Author

lidavidm added a commit that referenced this pull request Aug 11, 2022
lidavidm added a commit that referenced this pull request Aug 11, 2022
…13856)

This reverts commit e2efc87 (#13852) since it ended up with the wrong commit message.

Authored-by: David Li <[email protected]>
Signed-off-by: David Li <[email protected]>
lidavidm added a commit to lidavidm/arrow that referenced this pull request Aug 11, 2022
@ursabot
Copy link

ursabot commented Aug 11, 2022

Benchmark runs are scheduled for baseline = b7c94e2 and contender = e2efc87. e2efc87 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 ⬇️0.34% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.55% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.11% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] e2efc87f ec2-t3-xlarge-us-east-2
[Failed] e2efc87f test-mac-arm
[Failed] e2efc87f ursa-i9-9960x
[Finished] e2efc87f ursa-thinkcentre-m75q
[Finished] b7c94e2f ec2-t3-xlarge-us-east-2
[Failed] b7c94e2f test-mac-arm
[Failed] b7c94e2f ursa-i9-9960x
[Finished] b7c94e2f 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

@ursabot
Copy link

ursabot commented Aug 11, 2022

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
ursa-i9-9960x

@alamb
Copy link
Contributor

alamb commented Aug 11, 2022

I am sorry @lidavidm -- I think I got too excited about this improvement and just hit the Enticing green button. I should have reviewed the dev process again. 😞

Thanks for cleaning up my mess

@lidavidm
Copy link
Member Author

Ah, whoops! Well from Zulip apparently there's a setting we can ask Infra to toggle to also make that work, fortunately

@alamb
Copy link
Contributor

alamb commented Aug 11, 2022

I double checked and it seems like master has the change reverted (but not yet re-changed) https://github.com/apache/arrow/blob/master/dev/archery/archery/integration/runner.py#L433

Would it be helpful for me to to prepare a new PR with the change (revert the revert?)

@lidavidm
Copy link
Member Author

I put one up here already: #13858

Just haven't run the merge script yet

lidavidm added a commit that referenced this pull request Aug 11, 2022
…13858)

Redo the PR since the original ended up with the wrong commit message.

Authored-by: David Li <[email protected]>
Signed-off-by: David Li <[email protected]>
@lidavidm
Copy link
Member Author

And now it should be all good

@alamb
Copy link
Contributor

alamb commented Aug 11, 2022

Thanks again @lidavidm and sorry about that

ksuarez1423 pushed a commit to ksuarez1423/arrow that referenced this pull request Aug 15, 2022
) (apache#13858)

Redo the PR since the original ended up with the wrong commit message.

Authored-by: David Li <[email protected]>
Signed-off-by: David Li <[email protected]>
ksuarez1423 pushed a commit to ksuarez1423/arrow that referenced this pull request Aug 15, 2022
) (apache#13858)

Redo the PR since the original ended up with the wrong commit message.

Authored-by: David Li <[email protected]>
Signed-off-by: David Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flight] Upgrade to tonic > 0.3.1 to get fix of headers/trailers handling
4 participants