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: baseline spadev0 integration #28

Merged
merged 16 commits into from
Sep 11, 2023
Merged

feat: baseline spadev0 integration #28

merged 16 commits into from
Sep 11, 2023

Conversation

jcace
Copy link
Contributor

@jcace jcace commented Sep 4, 2023

Closes #24

Adds an integration to the Retrieval bot codebase to enable pulling CIDs from Spade and adding them to the task queue

integration/spadev0/main.go Outdated Show resolved Hide resolved
integration/spadev0/util.go Outdated Show resolved Hide resolved
integration/spadev0/util.go Outdated Show resolved Hide resolved
integration/spadev0/util.go Outdated Show resolved Hide resolved
for _, document := range replicas {
tasks = append(tasks, task.Task{
Requester: requester,
Module: task.HTTP, // TODO: Bitswap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xinaxu , do you think we should have an entirely separate Module here? ex task.Spade? Reason I ask is I don't see the metadata retrieve_type used anywhere (maybe I'm not looking in the right place though).. is is OK to use that metadata for control flow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Reason it is not used is because it is currently assuming there is only one type of retrieve_type and I'm expecting, as part of this PR or next PR, you will fork the logic, i.e., if retrieve_type is spade, do spade, otherwise, do regular.

Also, metadata will be part of the result payload, so you will be able to query the result collection with certain filtering on metadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

Meanwhile, please double check the current retrieval logic to make sure that it will not panic or crash on unexpected metadata/retrieve_type. You should be fine just by looking at your code

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, have you checked the logic here. This is where retrieval worker polls the task queue for tasks. If we end up filling the task but no-one is pulling from it, the task queue will get full and no new tasks will be added. I think you're fine by looking at your logic but want to make sure that you're aware.

if strings.HasPrefix(t.acceptedCountries, "!") {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meanwhile, please double check the current retrieval logic to make sure that it will not panic or crash on unexpected metadata/retrieve_type. You should be fine just by looking at your code

Yup, confirmed - the metadata retrieve_type is not actually read anywhere in the codebase, so should not cause any issues.

Finally, have you checked the logic here. This is where retrieval worker polls the task queue for tasks. If we end up filling the task but no-one is pulling from it, the task queue will get full and no new tasks will be added. I think you're fine by looking at your logic but want to make sure that you're aware.

should we add some kind of back-pressure check to decide to not run the integration if the queue is too full?

integration/spadev0/main.go Outdated Show resolved Hide resolved
@jcace jcace marked this pull request as ready for review September 7, 2023 02:37
@jcace jcace requested a review from xinaxu September 7, 2023 02:39
@jcace jcace self-assigned this Sep 7, 2023
Copy link

@ribasushi ribasushi left a comment

Choose a reason for hiding this comment

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

This is a very dcent first pass! The method is solid, left a few comments regarding implementation errors ( nothing architecturally bad though )

integration/spadev0/main.go Show resolved Hide resolved
integration/spadev0/main.go Outdated Show resolved Hide resolved
integration/spadev0/main.go Outdated Show resolved Hide resolved
.golangci.yml Show resolved Hide resolved
Copy link
Contributor

@xinaxu xinaxu left a comment

Choose a reason for hiding this comment

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

Please take a look at the comments before merging

integration/spadev0/util.go Outdated Show resolved Hide resolved
for _, document := range replicas {
tasks = append(tasks, task.Task{
Requester: requester,
Module: task.HTTP, // TODO: Bitswap
Copy link
Contributor

Choose a reason for hiding this comment

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

Reason it is not used is because it is currently assuming there is only one type of retrieve_type and I'm expecting, as part of this PR or next PR, you will fork the logic, i.e., if retrieve_type is spade, do spade, otherwise, do regular.

Also, metadata will be part of the result payload, so you will be able to query the result collection with certain filtering on metadata.

for _, document := range replicas {
tasks = append(tasks, task.Task{
Requester: requester,
Module: task.HTTP, // TODO: Bitswap
Copy link
Contributor

Choose a reason for hiding this comment

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

Meanwhile, please double check the current retrieval logic to make sure that it will not panic or crash on unexpected metadata/retrieve_type. You should be fine just by looking at your code

for _, document := range replicas {
tasks = append(tasks, task.Task{
Requester: requester,
Module: task.HTTP, // TODO: Bitswap
Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, have you checked the logic here. This is where retrieval worker polls the task queue for tasks. If we end up filling the task but no-one is pulling from it, the task queue will get full and no new tasks will be added. I think you're fine by looking at your logic but want to make sure that you're aware.

if strings.HasPrefix(t.acceptedCountries, "!") {

@jcace jcace merged commit 1d59f77 into main Sep 11, 2023
1 check failed
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.

v0 Spade CIDs - Retrieval Bot Integration
3 participants