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/update deps #309

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Chore/update deps #309

wants to merge 48 commits into from

Conversation

hwchen
Copy link
Collaborator

@hwchen hwchen commented Feb 28, 2021

Updating dependencies.

Major changes include:

  • clickhouse-rs to 1.0
  • actix-web to 4.0
  • async/await, futures to latest
  • tokio to 1.0

Ususally will only commit code that compiles, but this is a big
refactor.

So far:
- updated deps (use anyhow/thiserror over failure, update jsonwebtoken,
actix-web)
- udpated handler sigs (first wave of errors from actix-web)
- updated the config of the tesseract-server app

Now going back through handlers and fixing errors.
- update Backend trait in core to use async-trait crate
- update Backend trait in core to remove exec_sql_stream for now
- update clickhouse to 1.0.0-alpha.1
- udpate tesseract-clickhouse deps, remove failure for anyhow
- udpate tesseract-clickhouse tests to use tokio::test
- update tesseract-clickhouse to async/await syntax

After this, -core and -clickhouse should be working. Everything should
be broken, neither server nor other backends are updated yet.
@hwchen hwchen marked this pull request as draft February 28, 2021 16:47
@@ -49,29 +32,18 @@ fn test_query() {
// to ensure the SQL ingestion worked
let pool = Pool::new(database_url());
let sql = "SELECT month_name FROM tesseract_webshop_time;";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@frabarz wanted to ask about this table in the test. It seems to be hardcoded, without any initialization. I'm not quite sure why the test needed here, seems like it's testing something very specific in a workflow.

@hwchen
Copy link
Collaborator Author

hwchen commented Feb 28, 2021

@frabarz @alexandersimoes I came to a good stopping point to show some of the work.

I had started from the outside in for a bit (working on tesseract-server), but came to the point where I needed to go inside-out.

After the last two commits, core and clickhouse crates have been updated. You can see some of the types of changes I've made:

  • using async/await syntax
  • using async-trait for the Backend trait
  • updating error crates from failure to anyhow
  • silencing the exec_sql_stream method for now, since there's been substantial changes to futures::Stream
  • updating clickhouse tests to use tokio::test instead of a custom runner.

@hwchen
Copy link
Collaborator Author

hwchen commented Feb 28, 2021

btw, if you want to check out the changes to just clickhouse and core crates, you can run just the tests/compilation on those crates by running cargo from inside the directory, or specifying the crate in the workspace using command line args. That way you won't get all the errors from server.

- use async-trait
- use anyhow instead failure
- use std::future
- update to async/await syntax

update to lastes async_mysql:
- update rows_to_df
- update query_result.reduce
- use query_iter instead of prep_exec (I think this should be fine?)
- failure -> anyhow
- use async-trait
- update tokio (and some notes to remove it in future, or at least
runtime initialization)
e update bb8
- remove futures-state-stream

Updated syntax to async/await

Untested, but there's minimal changes it all compiles.
@hwchen
Copy link
Collaborator Author

hwchen commented Mar 6, 2021

Ok, postgres and mysql backends updated. I think that tomorrow I'll try to update everything in the server except for logic layer.

@alexandersimoes
Copy link

Thanks for all the updates @hwchen!

I tried running the tests like you recommended and running cargo test in the tesseract-core dir worked great, but I received a failed test for test test_query in the tesseract-clickhouse dir.

Here's a stacktrace:

Screen Shot 2021-03-09 at 8 23 27 PM

@alexandersimoes
Copy link

Reading a bit closer is this because I'm missing some tables in my local clickhouse installation? Specifically default.tesseract_webshop_time

Does running the tests also require a test db to be initd?

@hwchen
Copy link
Collaborator Author

hwchen commented Mar 10, 2021

That test is something I asked @frabarz about in another comment on the PR. I’d like to figure out if it’s specific to some deployment

@frabarz
Copy link
Contributor

frabarz commented Mar 10, 2021

Sorry, didn't see the past comment.

AFAIK, Jonathan wrote these tests. These are configured to run against a clickhouse database, stored in a docker container with a copy of the https://github.com/tesseract-olap/tesseract-example-app repository.

It's just for CI. The travis config has the detail of the process in the before_install key, but to summarize, it runs the ./docker-compose.yml recipe, which in turn pulls the yandex/clickhouse-server:19.3 image to create a container, and runs the ./docker/scripts/init_db.sh script inside it.

@alexandersimoes
Copy link

Ah ok, I didn't see that other comment.

@hwchen
Copy link
Collaborator Author

hwchen commented Mar 22, 2021

Chipping away. Was wondering, does any instance of tesseract use the streaming option? (which allows streaming of http responses). If not, I'm going to comment it out for a bit and deal with it later.

@alexandersimoes
Copy link

For now we are not using this release candidate anywhere anyway, so there is no problem with commenting out the streaming option for now.

@hwchen hwchen marked this pull request as ready for review October 11, 2021 13:12
@hwchen
Copy link
Collaborator Author

hwchen commented Oct 11, 2021

@alexandersimoes deps update is complete. I'd like to do some integration testing, please see the note I left you by email.

After this PR is in, I'll probably start with updating the test suite. One test doesn't pass locally, I want to make sure it's integrated correctly.

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.

3 participants