Skip to content

Conversation

frrist
Copy link
Member

@frrist frrist commented Jun 22, 2022

@frrist frrist self-assigned this Jun 22, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1001 (307a24b) into frrist/update-lotusv1.16.0-rc (9a67cff) will decrease coverage by 0.9%.
The diff coverage is 0.0%.

@@                       Coverage Diff                       @@
##           frrist/update-lotusv1.16.0-rc   #1001     +/-   ##
===============================================================
- Coverage                           34.8%   33.8%   -1.0%     
===============================================================
  Files                                 31      32      +1     
  Lines                               2125    2186     +61     
===============================================================
  Hits                                 741     741             
- Misses                              1298    1359     +61     
  Partials                              86      86             

@hsanjuan
Copy link
Contributor

Why not migrating the current table to have one extra column (IsString), and start using that column from now on?

When reprocessing the chain then we will get the field correctly populated (we don't have to do it now).

Having two tables is a pain. We always say that we are going to add views and what not, but we don't, it complicates the exporting etc.

Given that we have to do a full chain reprocessing soon (where we can go to having 1 table only again), it would be nice to not have 2 tables in the meantime.

@frrist
Copy link
Member Author

frrist commented Jun 23, 2022

Agreed, having two tables is complicated. Previously when adding a migration similar to this we did so to prevent breaking the archive process. But since we are planning to do a full chain reprocess soon I'd be open to making this an addition to the existing table schema. cc @kasteph

@frrist frrist requested review from hsanjuan and kasteph June 23, 2022 15:00
@frrist
Copy link
Member Author

frrist commented Jun 23, 2022

@hsanjuan and @kasteph could you share more of your thoughts here? Right now this PR has created a new model/table. But if were wanting to go a different route we'll need to know before the week is over.

@frrist frrist force-pushed the frrist/update-lotusv1.16.0-rc branch from 41f384a to d5162b5 Compare June 29, 2022 01:54
@hsanjuan
Copy link
Contributor

@hsanjuan and @kasteph could you share more of your thoughts here? Right now this PR has created a new model/table. But if were wanting to go a different route we'll need to know before the week is over.

Iirc, single table should be fine. Add one extra column but don't populate it for the things in the past, and start using the new format on the existing label column too (no need to base64 all existing labels in all existing rows).

@frrist frrist force-pushed the frrist/update-lotusv1.16.0-rc branch 2 times, most recently from 6d8db02 to 1da0a11 Compare June 29, 2022 21:15
@frrist
Copy link
Member Author

frrist commented Jun 30, 2022

closing in favor of #1015

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