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: skip sqlserver+flight combination. #2526

Merged
merged 26 commits into from
Jan 30, 2024
Merged

chore: skip sqlserver+flight combination. #2526

merged 26 commits into from
Jan 30, 2024

Conversation

tychoish
Copy link
Collaborator

@tychoish tychoish commented Jan 30, 2024

This (on top of #2518) should get wall clock time to about 10 minutes for an entire PR run, at the same cost.

It's likely redundant to the other suite. We could also switch datasource integration to just postgres as there's less postgres protocol coverage.

@tychoish
Copy link
Collaborator Author

Screenshot Capture - 2024-01-30 - 01-38-17

Not nothing. This is almost best case (good caches,) there were some scheduling delays, but we're mostly hitting the "compile+bindings" DAG limitation (which means there's another 4ish minutes of wall clock time that we could get.

The runner is one of the small ones so this is budget neutral, but this will push build times from 10-15 minutes into 5-10 minutes.

@vrongmeal
Copy link
Contributor

on top of #2949

I think you tagged a wrong issue/PR


Why this change though? Is it that Flight SQL is still in development so we can afford to skip some checks for now that take very long?

Base automatically changed from tycho/ci-caching-and-runner-size to main January 30, 2024 15:00
@tychoish
Copy link
Collaborator Author

Why this change though? Is it that Flight SQL is still in development so we can afford to skip some checks for now that take very long?

It takes 4 or 5 minutes (there are a lot of CI PRs up at the moment, so the logic is a bit distributed,) to run the SQL server tests (which we were doing twice, for each protocol). Particularly since flightsql isn't deployed or used at the moment, this feels like a lot of time to be spent on tests like these. While I think it's important to run a lot of queries against different protocols and with different clients, I don't know that the matrix of combinations of protocols and data sources is going to give us a lot of signal (it might find things accidentally, but that doesn't feel like it'd be worth the wait time and we can have a stronger test strategy.

@tychoish tychoish closed this Jan 30, 2024
@tychoish tychoish reopened this Jan 30, 2024
@tychoish
Copy link
Collaborator Author

Did a bit of buddy-system investigation with @scsmithr:

  • each run of the sqlserver tests takes 4m30s,
  • the large_table tablescan is the slow thing. we do it four times: each run has a count and a full scan and we do the full run against different protocols.
  • Pushing down the count will probably be the biggest win here, and then we'd be able to (if we want), run flight+rpc in parallel, which would make sqlserver tests run in about as much time as the longer of the other datasource tests.
  • the fixtures for setting up the test (the shell script) is slower than it should be, there's some speed gains there.
  • there is an errant print statement in th sql server implementation.

@tychoish tychoish merged commit e4ecdce into main Jan 30, 2024
21 checks passed
@tychoish tychoish deleted the tycho/ci-sql-server branch January 30, 2024 19:29
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.

None yet

3 participants