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

Add missing score on peer_score table #2

Merged
merged 2 commits into from
May 23, 2023

Conversation

cortze
Copy link

@cortze cortze commented Apr 21, 2023

Description

The peer_score_event table seems to be missing the actual Score field, as suggested in #1 .

I'm trying to test it locally, but since my local lotus uses the BulkIndexer, there seems to be some incompatibility with tracecatcher's HTTP server request handler (I'll figure out how to solve it).

Copy link
Contributor

@iand iand left a comment

Choose a reason for hiding this comment

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

The change is correct but it's not backwards compatible. We have a few options:

  1. drop the old table and data and recreate with this new schema
  2. implement a migration system that will add the column to existing table, though it can't be NOT NULL in that case
  3. create a new table peer_score_event_v2

My preference is to do option 3. It allows us to keep the historic data we collected and have the exact table schema we want. It also allows us to vary the schema again in the future if we need to.

Can you change the name of the table everywhere it is mentioned to peer_score_event_v2

@cortze
Copy link
Author

cortze commented May 22, 2023

Hey @iand ! I'm totally fine with option 3, let me address it in a new commit

Copy link
Contributor

@iand iand left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for working on this

@iand iand merged commit 541c58a into probe-lab:main May 23, 2023
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