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

chore: move slt runner into main binary #2401

Merged
merged 21 commits into from
Jan 13, 2024
Merged

chore: move slt runner into main binary #2401

merged 21 commits into from
Jan 13, 2024

Conversation

tychoish
Copy link
Contributor

Addresses #2378

See discussion on the ticket.

Copy link
Contributor

@universalmind303 universalmind303 left a comment

Choose a reason for hiding this comment

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

I won't be a blocker for this, but I'd still much prefer feature flagging instead of just hiding. While the extra binary size isn't a lot, every little bit slowly adds up.

Since it is exclusively a development feature, it seems natural to feature flag as such.

@tychoish
Copy link
Contributor Author

I won't be a blocker for this, but I'd still much prefer feature flagging instead of just hiding. While the extra binary size isn't a lot, every little bit slowly adds up.

by my read this shouldn't increase the binary by more than a few kb

Since it is exclusively a development feature, it seems natural to feature flag as such.

let's get it working first 😂 and talk about it on friday.

@tychoish
Copy link
Contributor Author

Sorry for the really noisy PR, I've been trying to iron and and do a bit of cleanup with the dependencies just in awareness of the binary size. My conclusion generally:

  • it's probably the flightsql client (this is the main thing that's in slt that wasn't in glaredb before.
  • we might get more out of doing some flightsql RPC tests using a non-rust client (because testing it with go or python might reveal something tonic-specific that we've done, that the existing tests wouldn't catch.)
  • the flightsql client will become moot when we start using flightsql as our internal IPC for dist exec anyway, so it's not super important now.
  • we can always flag this out of the build later if we want, but I'd like to start with giving it a try.
  • I think the most compelling argument for keeping this in main is that it means we can build glaredb for aarch64 and then run the SLTs with the binary from the package on the actual hardware, rather than just trusting that all of the cross compilation works.

Do folks agree?

@tychoish tychoish merged commit 4590005 into main Jan 13, 2024
18 of 19 checks passed
@tychoish tychoish deleted the tycho/slt-in-main branch January 13, 2024 03:52
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.

2 participants