Skip to content

fix: rearrange pieces in sector for no padding #306

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

Merged
merged 3 commits into from
Nov 2, 2024
Merged

Conversation

LexLuthr
Copy link
Contributor

No description provided.

@LexLuthr LexLuthr requested a review from magik6k October 28, 2024 16:01
Copy link
Collaborator

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

I'd like to somehow avoid the added state in the DB, any added thing there will make interactions more complex as it's another thing that needs to be kept in sync with everything.

@@ -0,0 +1,171 @@
CREATE TABLE deal_index_tracker (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this instead of looking e.g. at the _initial_pieces tables and the piece_index there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't a big fan of this approach while I was writing it. I was trying to find some way to avoid state but could not think of a reliable way which did not involve some really long function. Here is the problem:

  1. Typically we can easily identify the piece with piece CID and Size.
  2. Problem starts when we have the same piece in more than 1 location in the same sector. How do I reliably connect a piece to a deal. We don't even need to index it later but we still need to know about the offset. This was a reason I went with a stateful solution.

Another approach I can think of would be to add an index_column to market_mk12_deal_pipeline table and populate it during the transfer functions from within the SQL. It seems cheaper than the current solution.

If you have any insight which avoids any of this, I can really use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Slack:
We are not going to allow same piece more than once in a sector. This removes the stateful tracking to find unique pieces and their offset in sector.

Copy link
Collaborator

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Don't see anything wrong here, we'll need to make sure to test all this quite well though

@magik6k magik6k merged commit 90857d7 into feat/market Nov 2, 2024
14 checks passed
@magik6k magik6k deleted the piece-padding branch November 2, 2024 17:41
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