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

indexer management client with db connection and cost server #43

Merged
merged 5 commits into from Sep 5, 2023

Conversation

hopeyen
Copy link
Collaborator

@hopeyen hopeyen commented Aug 31, 2023

Resolves #19 and #3
some quick notes

  • Simple indexer management client is now tracking both NetworkSubgraph and postgres connection
  • Define cost model schema and resolvers with postgres and graphQL types, with global cost model fallback when specific deployments are queried
  • No migration in indexer service; indexer agent is solely responsible for database management.

@hopeyen hopeyen self-assigned this Aug 31, 2023
@hopeyen hopeyen force-pushed the hope/indexer-management-client branch from e796ba0 to ea26a77 Compare August 31, 2023 00:24
@hopeyen hopeyen changed the title indexer management client with database connection and cost model service indexer management client with db connection and cost server Aug 31, 2023
@hopeyen hopeyen force-pushed the hope/indexer-management-client branch from ea26a77 to 56d2c51 Compare August 31, 2023 00:27
@github-actions
Copy link

github-actions bot commented Aug 31, 2023

Pull Request Test Coverage Report for Build 6053871226

  • 0 of 226 (0.0%) changed or added relevant lines in 8 files are covered.
  • 3 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-3.8%) to 37.705%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/src/common/indexer_error.rs 0 2 0.0%
service/src/server/mod.rs 0 2 0.0%
service/src/main.rs 0 10 0.0%
service/src/server/routes/cost.rs 0 10 0.0%
service/src/common/indexer_management_client/mod.rs 0 12 0.0%
service/src/common/database.rs 0 21 0.0%
service/src/common/indexer_management_client/schema.rs 0 26 0.0%
service/src/common/indexer_management_client/resolver.rs 0 143 0.0%
Files with Coverage Reduction New Missed Lines %
service/src/common/database.rs 1 0.0%
service/src/metrics/mod.rs 1 0.0%
service/src/util.rs 1 17.35%
Totals Coverage Status
Change from base Build 6029635279: -3.8%
Covered Lines: 874
Relevant Lines: 2318

💛 - Coveralls

@hopeyen hopeyen force-pushed the hope/indexer-management-client branch from 6bb57b8 to e30e5cc Compare August 31, 2023 01:24
@hopeyen hopeyen added size:large Large p2 Medium priority type:feature New or enhanced functionality labels Aug 31, 2023
@hopeyen hopeyen requested a review from aasseman August 31, 2023 13:26
@hopeyen hopeyen force-pushed the hope/indexer-management-client branch from e30e5cc to 05ca6c0 Compare August 31, 2023 13:31
@hopeyen hopeyen force-pushed the hope/indexer-management-client branch from 05ca6c0 to 3ccc617 Compare August 31, 2023 13:46
@aasseman aasseman linked an issue Aug 31, 2023 that may be closed by this pull request
4 tasks
Copy link
Collaborator

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

LGTM overall, just a few little things (see code comments).

Things that could be explored later:

  • It'd be nice to have some tests on the graphql server, with request/response test vectors.
  • Perhaps some migrations could be added only for the purposes of testing. I couldn't build on my machine (sqlx complaining) and had to run a PG instance and create the table myself. Though I'm confused why the CI didn't run into that problem.
  • Related to the point above, we could set-up the CI such that there is a PG instance available, to avoid ignoring all the tests using sqlx.

service/src/common/indexer_management_client/resolver.rs Outdated Show resolved Hide resolved
@hopeyen hopeyen force-pushed the hope/indexer-management-client branch 2 times, most recently from 6d33a3d to ad3ed6c Compare September 1, 2023 14:34
@hopeyen
Copy link
Collaborator Author

hopeyen commented Sep 1, 2023

Thanks for the review @aasseman !
I've addressed the comments and regarding to things for later

tests on the graphql server, with request/response test vectors.

Agreed and added an item on #15

Perhaps some migrations could be added only for the purposes of testing. I couldn't build on my machine (sqlx complaining) and had to run a PG instance and create the table myself. Though I'm confused why the CI didn't run into that problem.

Would it be because those tests had [ignore] and cargo llvm-cov test doesn't test the ignored?
I've added a migrations folder for you to try out if you would like

Related to the point above, we could set-up the CI such that there is a PG instance available, to avoid ignoring all the tests using sqlx.

Agreed as well, it would just need to run sqlx migrate run once the instance is available

Copy link
Collaborator

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

One last thing, perhaps adding a migrations/README.md that explains that the migrations are there only for testing?

@aasseman
Copy link
Collaborator

aasseman commented Sep 1, 2023

FYI #44

@hopeyen hopeyen force-pushed the hope/indexer-management-client branch from ad3ed6c to 478182b Compare September 1, 2023 20:50
@hopeyen
Copy link
Collaborator Author

hopeyen commented Sep 1, 2023

adding a migrations/README.md that explains that the migrations are there only for testing

Good idea! added in 478182b

Copy link
Collaborator

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

:shipit:

@hopeyen hopeyen merged commit 12c7ef1 into main Sep 5, 2023
5 checks passed
@hopeyen hopeyen deleted the hope/indexer-management-client branch September 5, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 Medium priority size:large Large type:feature New or enhanced functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexer management client feat: Cost server
2 participants