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

THRIFT-5648: add uuid support in rust #2695

Merged
merged 3 commits into from
Oct 12, 2023
Merged

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented Oct 8, 2022

  • 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.

@jimexist jimexist requested a review from allengeorge October 8, 2022 06:29
@jimexist jimexist changed the title rust to add uuid support THRIFT-5648: rust to add uuid support Oct 8, 2022
lib/rs/Cargo.toml Outdated Show resolved Hide resolved
@jimexist jimexist force-pushed the rs-uuid branch 2 times, most recently from 999bac2 to 5af48d7 Compare October 8, 2022 09:36
@jimexist jimexist requested a review from Jens-G October 8, 2022 09:37
@jimexist jimexist changed the title THRIFT-5648: rust to add uuid support THRIFT-5648: add uuid support in rust Oct 9, 2022
@jimexist
Copy link
Member Author

jimexist commented Oct 9, 2022

@allengeorge per discussion in #2700, @fishy suggested that we do not bring any 3rd dependencies (in go library). here we are facing the same issue, do you believe we shall remove the uuid dependency in rust?

@fishy
Copy link
Member

fishy commented Oct 9, 2022

@allengeorge per discussion in #2700, @fishy suggested that we do not bring any 3rd dependencies (in go library). here we are facing the same issue, do you believe we shall remove the uuid dependency in rust?

go library is slightly different in that it already has zero third party dependency right now so there's a big incentive to keep it that way, which does not apply to rust library (which I believe already have third party dependencies).

@allengeorge
Copy link
Contributor

allengeorge commented Oct 12, 2022

I'm ok with adding a dependency (we shouldn't add them without thought, of course). The Rust UUID crate is dual-licensed ASL 2.0/MIT, so that's not an issue.

We can always reimplement the UUID portions necessary and remove the dependency over time.

@jimexist
Copy link
Member Author

I'm ok with adding a dependency (we shouldn't add them without thought, of course). The Rust UUID crate is dual-licensed ASL 2.0/MIT, so that's not an issue.

We can always reimplement the UUID portions necessary and remove the dependency over time.

thanks for the context. @allengeorge in this case do you think this pr can be merged as is?

@Jens-G Jens-G added the rust label Oct 25, 2022
@Jens-G Jens-G removed their request for review October 25, 2022 20:30
@jimexist jimexist force-pushed the rs-uuid branch 2 times, most recently from 9d5c148 to f885a18 Compare October 27, 2022 02:16
@jimexist
Copy link
Member Author

jimexist commented Nov 22, 2022

hi @allengeorge and @Jens-G please take a look at this, thanks!

@jimexist jimexist closed this May 3, 2023
@my-vegetable-has-exploded

I'd like to know why this pr was closed. thanks

@jimexist
Copy link
Member Author

I'd like to know why this pr was closed. thanks

hi, simple answer - i was not able to get approvers for this.

@my-vegetable-has-exploded

I'd like to know why this pr was closed. thanks

hi, simple answer - i was not able to get approvers for this.

what a pity!

@Jens-G
Copy link
Member

Jens-G commented Sep 26, 2023

hi, simple answer - i was not able to get approvers for this.

For starters, I am not a Rust guy, and @allengeorge already chimed in. That's why I removed the review request.

Next, we have plenty of pending PRs, which is a whole topic on itself. Getting no approvers applies to most of them, but the real question is why? IMHO this is not a indication of a quality issue, but a quantity issue. If we had enough active committers taking care, we would be able to give better responses than "there just was nobody around to take care".

@Jens-G
Copy link
Member

Jens-G commented Sep 26, 2023

I just skimmed over the changes and what could be better is the fact, that the compiler part also has a lot of refactorings in it. It's a bit hard to see what the real changes are for uuid and what is just shuffling comments around etc. I'd say we give it another chance. Shall we?

@Jens-G Jens-G reopened this Sep 26, 2023
@Jens-G Jens-G merged commit df0c674 into apache:master Oct 12, 2023
16 of 24 checks passed
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.

5 participants