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

feat: manual publish storage deal message #1585

Merged
merged 12 commits into from
Jul 24, 2023
Merged

feat: manual publish storage deal message #1585

merged 12 commits into from
Jul 24, 2023

Conversation

LexLuthr
Copy link
Collaborator

@LexLuthr LexLuthr commented Jul 20, 2023

Current Scenario

Currently, Boost controls when the PSD message is sent for a deal. This decision is governed by a couple of factors.

  1. A timeout value forcing to publish all deals (1 hour)
  2. Maximum number of deal to be included in each message (8)
    Another special case is to Publish all deals forcefully using UI button and Graphql mutation.

Feature request background

SPs have expressed interest in being able to control how the deal are published. The reason this control ranges from being able to control deal flow to miner to control the cost of PSD message

Feature

The new feature allows SPs to turn on manual PSD. Once this feature is turned on, Boost will no longer send any PSD messages unless explicitly prompted by the user. The feature can be turned on with the below config variable:

[Dealmaking]
        // When set to true, the user is responsible for publishing deals manually.
	// The values of MaxDealsPerPublishMsg and PublishMsgPeriod will be
	// ignored, and deals will remain in the pending state until manually published.
	ManualDealPublish bool

Querying the deals pending to be published

The deals which have not been published can be queried using the Graphql endpoint of the Boost. The curl for the query:

curl -X POST -H "Content-Type: application/json" -d '{"query":"query { dealPublish{ ManualPSD Deals {ID IsLegacy ClientAddress ProviderAddress CreatedAt PieceCid PieceSize ProviderCollateral StartEpoch EndEpoch ClientPeerID PublishCid Transfer { Type Size Params ClientID} Message }}}"}' http://localhost:8080/graphql/query | jq

Decision

SPs can create a decision script which queries the Graphql endpoint using the above curl and take a decision on which deal to be published.

Publishing deals

Once the decision has been taken on which deals to be published, SPs can use the Grapqhql endpoint to Publish the deals.
The mutation is:

publishPendingDeals(ids: [ID!]!): [ID!]!

which takes an array of deal UUIDs to be published and an error message is returned. The following curl can be used to publish the deal.

curl -X POST -H "Content-Type: application/json" -d '{"query":"mutation {publishPendingDeals(ids: [\"d9f849f1-d5d8-4bfc-b034-2866bddfc8cb\", \"a8eb58ef-7381-4251-ae7a-1227c032c0b9\"])}"}' http://localhost:8080/graphql/query | jq

Successful output

{
  "data": {
    "publishPendingDeals": [
      "d9f849f1-d5d8-4bfc-b034-2866bddfc8cb",
      "a8eb58ef-7381-4251-ae7a-1227c032c0b9"
    ]
  }
}

Failure output

{
  "errors": [
    {
      "message": "failed to get deal details from DB cc78d877-f69c-410b-80a7-b23f2fab8951: scanning deal row: sql: no rows in result set",
      "path": [
        "publishPendingDeals"
      ]
    }
  ],
  "data": null
}

Publish all deals

The publish all button in UI would have the same functionality as before. Same would be true for the Graphql mutation dealPublishNow

UI screenshots

Screenshot 2023-07-19 at 11 09 14 AM

markets/storageadapter/dealpublisher.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher.go Show resolved Hide resolved
Comment on lines 497 to 498
// Remove the deal from Pending
p.cleanupPending(deals)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we clean up pending deals under the lock also, so that the whole operation is atomic.
Otherwise two threads could interfere with each other.
Maybe you can even refactor so that it splits the list into the items to be published and the items that will remain (you're doing this in separate for loops at the moment, probably you can do both in the same for loop).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have refactored the code a little. Unique identifier don't go well with slice.
The new code looks more clean. It resulted in some minor changes in other functions as well.

@@ -232,6 +233,61 @@ func TestForcePublish(t *testing.T) {
checkPublishedDeals(t, dpapi, dealsToPublish, []int{2})
}

func TestPublishPendingDeals(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a test that only publishes some of the deals instead of all of them.
eg there are three pending deals, only publish two of them and verify that there is one remaining pending deal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have refactored the test to send 4 deals. 1 with ctx.Error and other 3 normal. It will try to Publish 3 deals total (2 correct ones and 1 random CID which is not in Publisher). Test will result in 2 deal Publish, 1 debug log with error about random CID (not found in publisher) and returning 1 pending deal.

@LexLuthr LexLuthr requested a review from dirkmc July 21, 2023 08:39
markets/storageadapter/dealpublisher.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher_test.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher_test.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher_test.go Outdated Show resolved Hide resolved
@LexLuthr LexLuthr requested a review from dirkmc July 24, 2023 08:01
@LexLuthr LexLuthr merged commit 53fdf99 into main Jul 24, 2023
@LexLuthr LexLuthr deleted the manual-psd-1 branch July 24, 2023 15:11
@LexLuthr LexLuthr mentioned this pull request Aug 3, 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