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

Check for new added nodes #512

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Conversation

AbdelrahmanElawady
Copy link
Collaborator

Description

Check for new added nodes periodically and update GPU data when new nodes are added.

Related Issues

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring

@@ -29,6 +30,7 @@ type NodeGPUIndexer struct {
batchSize int
nodesGPUResultsChan chan []types.NodeGPU
nodesGPUBatchesChan chan []types.NodeGPU
newNodeTwinIDChan chan []uint32
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that a channel of int32 slice? this should keep receiving the latest twinID inserted on the nodes table, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah forget it checks the nodes between the last id you checked and the last id on the table, could be more than one indeed

@@ -934,3 +934,9 @@ func (p *PostgresDatabase) UpsertNodesGPU(ctx context.Context, nodesGPU []types.
}
return nil
}

func (p *PostgresDatabase) GetLastNodeTwinID(ctx context.Context) (uint32, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should change it to be GetGreaterThanTwinID and use the twinID we checked last? this should get us the list of ids that we can directly send?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially, we thought we only need the latest ID and all new nodes will be from last checked to the latest ID however this is not right as twin IDs are shared with other entities so node twin IDs don't have to be incremented one by one. So, this approach is probably better.

ticker := time.NewTicker(newNodesCheckInterval)
latestCheckedID, err := n.db.GetLastNodeTwinID(ctx)
if err != nil {
log.Error().Err(err).Msg("failed to get last node twin id")
Copy link
Contributor

Choose a reason for hiding this comment

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

This means the proxy is having a seriou issue where there're no nodes in the table? or could that while the pod is starting for the first time?Is that the reason why we don't panic here?

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 think there might be non-fatal errors here that would not stop the operation of the indexer, that's why we don't panic. What I can think of is maybe some network issue or db connection error on start. I think we don't need to panic here.

n.newNodeTwinIDChan <- nodeTwinIDs
latestCheckedID = int64(nodeTwinIDs[0])
case <-ctx.Done():
break
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be return?

@xmonader xmonader merged commit 62c09a7 into development Nov 28, 2023
19 checks passed
@xmonader
Copy link
Contributor

Thank you 💯

@AbdelrahmanElawady AbdelrahmanElawady deleted the development_improve_gpu_indexer branch November 28, 2023 23:35
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