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.

Sign up for GitHub

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

Jump to bottom

THRIFT-5564: add a rust GitHub action and cross test [skip ci] #2632

Merged
merged 1 commit into from
Oct 8, 2022
Merged

THRIFT-5564: add a rust GitHub action and cross test [skip ci] #2632

merged 1 commit into from
Oct 8, 2022

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented Jul 4, 2022 •
edited

as it replaces both

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

allengeorge
allengeorge reviewed Jul 7, 2022
View reviewed changes
Copy link
Contributor

@allengeorge allengeorge left a comment

Choose a reason for hiding this comment

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

Reviewed everything but the github actions. In general this looks great! Were you able to run the cross tests successfully?

lib/rs/src/protocol/compact.rs Outdated
let possible_element_count = (header & 0xF0) >> 4;
if possible_element_count != 15 {
let element_count = if possible_element_count != 15 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh - didn't know you could do that now.

test/rs/src/bin/test_client.rs Outdated
@@ -492,8 +483,8 @@ fn make_thrift_calls(
s_cmp_nested_2.insert(Numberz::SIX, empty_insanity);

let mut s_cmp: BTreeMap<UserId, BTreeMap<Numberz, Insanity>> = BTreeMap::new();
s_cmp.insert(1 as UserId, s_cmp_nested_1);
s_cmp.insert(2 as UserId, s_cmp_nested_2);
s_cmp.insert(1_i64, s_cmp_nested_1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you leave these two as before please?

Copy link
Contributor

@OmmyZhang OmmyZhang Jul 18, 2022 •
edited

Choose a reason for hiding this comment

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

@allengeorge 1 as UserId will cause clippy::unnecessary_cast

error: casting integer literal to `i64` is unnecessary

test/rs/src/bin/test_client.rs Outdated
s_cmp.insert(1 as UserId, s_cmp_nested_1);
s_cmp.insert(2 as UserId, s_cmp_nested_2);
s_cmp.insert(1_i64, s_cmp_nested_1);
s_cmp.insert(2_i64, s_cmp_nested_2);
Copy link
Contributor

Choose a reason for hiding this comment

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

And this?

lib/rs/src/server/multiplexed.rs Outdated
.invoked
.compare_and_swap(false, true, Ordering::Relaxed);
if res {
let res =
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting: I had no idea this was deprecated now.

lib/rs/test/src/bin/kitchen_sink_client.rs Outdated
@@ -33,7 +33,6 @@ use kitchen_sink::recursive;
use kitchen_sink::recursive::{CoRec, CoRec2, RecList, RecTree, TTestServiceSyncClient};
use kitchen_sink::ultimate::{FullMealServiceSyncClient, TFullMealServiceSyncClient};

use thrift;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we don't have to include the use definition here?

test/rs/src/bin/test_server.rs Outdated
@@ -16,14 +16,12 @@
// under the License.

use clap::{clap_app, value_t};
use env_logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little surprised that we don't need the use declaration anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

@allengeorge this is clippy::single_component_path_imports

error: this import is redundant

test/rs/src/lib.rs Outdated
@@ -15,5 +15,11 @@
// specific language governing permissions and limitations
// under the License.

// FIXME - need changes in gen before lifting this exception
#![allow(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you create a followup JIRA for this?

@allengeorge
Copy link
Contributor

Unfortunately it looks like all the rust cross-tests are failing in the "Build/cross-test" action - even the rust<->rust ones, which should always work.

@OmmyZhang OmmyZhang mentioned this pull request Jul 17, 2022
Closed
5 tasks
@jimexist jimexist changed the title rust to use 1.61 toolchain and combine with rust GitHub action add a rust GitHub action Sep 3, 2022
@jimexist jimexist changed the title add a rust GitHub action add a rust GitHub action and cross test Sep 3, 2022
@jimexist jimexist mentioned this pull request Sep 3, 2022
Closed
5 tasks
@jimexist jimexist force-pushed the rs-action-161 branch 2 times, most recently from 07f4936 to 8f700dc Compare September 28, 2022 00:33
@jimexist jimexist changed the title add a rust GitHub action and cross test add a rust GitHub action and cross test [skip ci] Sep 28, 2022
@jimexist jimexist requested review from allengeorge and OmmyZhang and removed request for OmmyZhang September 28, 2022 07:38
@jimexist
Copy link
Member Author

jimexist commented Oct 8, 2022

i'm merging this given the failure is due to missing uuid impl in rust:

testUuid("00112233-4455-6677-8899-aabbccddeeff")
*** FAILURE ***
org.apache.thrift.TApplicationException: unknown method testUuid
	at org.apache.thrift.TServiceClient.receiveBase(TServiceClient.java:81)
	at thrift.test.ThriftTest$Client.recv_testUuid(ThriftTest.java:527)
	at thrift.test.ThriftTest$Client.testUuid(ThriftTest.java:514)
	at org.apache.thrift.test.TestClient.main(TestClient.java:257)
Min time: 0us
Max time: 0us
Avg time: 0us

@jimexist jimexist merged commit 0aad2ae into apache:master Oct 8, 2022
@jimexist jimexist deleted the rs-action-161 branch October 8, 2022 05:22
@jimexist jimexist changed the title add a rust GitHub action and cross test [skip ci] THRIFT-5564: add a rust GitHub action and cross test [skip ci] Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmmyZhang OmmyZhang OmmyZhang left review comments

@allengeorge allengeorge Awaiting requested review from allengeorge

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jimexist @allengeorge @OmmyZhang