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(torch): v1 resources added #2

Merged
merged 45 commits into from
Oct 30, 2023
Merged

feat(torch): v1 resources added #2

merged 45 commits into from
Oct 30, 2023

Conversation

tty47
Copy link

@tty47 tty47 commented Oct 23, 2023

Overview

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
@tty47 tty47 requested a review from a team October 24, 2023 16:39
@tty47 tty47 self-assigned this Oct 24, 2023
@tty47 tty47 added enhancement New feature or request fix Fixes labels Oct 24, 2023
Signed-off-by: Jose Ramon Mañes <[email protected]>
@tty47 tty47 marked this pull request as ready for review October 25, 2023 10:16
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

If you still want to use context.TODO() - please give a very comprehensive reasoning why instead of other type of contexts 🙏

Great job so far!

Dockerfile Outdated Show resolved Hide resolved
Dockerfile_local Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
pkg/db/redis/manager.go Outdated Show resolved Hide resolved
pkg/db/redis/manager.go Show resolved Hide resolved
pkg/http/handlers.go Outdated Show resolved Hide resolved
pkg/http/server.go Outdated Show resolved Hide resolved
pkg/nodes/queue.go Outdated Show resolved Hide resolved
Comment on lines 21 to 36
func ProcessTaskQueue() {
ticker := time.NewTicker(5 * time.Second) // Set the interval to 5 seconds
//defer ticker.Stop() // Stop the ticker when the function exits

for {
select {
case <-ticker.C:
processQueue()
}
}
}

// processQueue process the nodes in the queue and tries to generate the Multi Address
func processQueue() {
red := redis.InitRedisConfig()
ctx := context.TODO()
Copy link
Member

Choose a reason for hiding this comment

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

As a good practice you need to pass context downstream so you can have a select case if the context is either done or not

Copy link
Author

Choose a reason for hiding this comment

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

I've updated these functions, now they look like:

// ProcessTaskQueue processes the pending tasks in the queue the time specified in the const TickerTime.
func ProcessTaskQueue(ctx context.Context) {
	ticker := time.NewTicker(TickerTime)

	for {
		select {
		case <-ctx.Done():
			// The context has been canceled, exit the loop.
			return
		case <-ticker.C:
			processQueue(ctx)
		}
	}
}

// processQueue process the nodes in the queue and tries to generate the Multi Address
func processQueue(ctx context.Context) {
	red := redis.InitRedisConfig()

	for {
		select {
		case <-ctx.Done():
			// The context has been canceled, exit the loop.
			return
		case peer := <-taskQueue:
			// Perform the operation with the node
			err := CheckNodesInDBOrCreateThem(peer, red, ctx)
			if err != nil {
				log.Error("Error checking the nodes: CheckNodesInDBOrCreateThem - ", err)
			}
		}
	}
}

is this the right usage as you mentioned?

pkg/nodes/queue.go Outdated Show resolved Hide resolved
tty47 and others added 8 commits October 26, 2023 10:05
Co-authored-by: Nguyen Nhu Viet <[email protected]>
…therwise it will generate it

Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
@tty47 tty47 requested review from Bidon15 and a team October 26, 2023 13:45
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

🖖 another round please 🙏

pkg/nodes/queue.go Show resolved Hide resolved
pkg/nodes/queue.go Outdated Show resolved Hide resolved
}

// processQueue process the nodes in the queue and tries to generate the Multi Address
func processQueue() {
Copy link
Member

Choose a reason for hiding this comment

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

I highly recommend you return errors up the call stack as the never ending loop of calls based on ticker and no exit is bad design pick

Comment on lines +24 to +28
for {
select {
case <-ticker.C:
processQueue()
}
Copy link
Member

Choose a reason for hiding this comment

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

Ideally here you need to process errors returned from processQueue()
Also, you need to return a general error or nil error in ProcessTaskQueue() func as it is a good practice

pkg/nodes/queue.go Outdated Show resolved Hide resolved
Comment on lines 14 to 15
var (
taskQueue = make(chan config.Peer) // taskQueue channel for pending tasks (peers to process later).
Copy link
Member

Choose a reason for hiding this comment

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

it's not good to do taskQueue like this.

You can either create a struct that holds the queue functions or only create it in AddPhase

pkg/nodes/nodes.go Show resolved Hide resolved
pkg/nodes/nodes.go Outdated Show resolved Hide resolved
log.Info("MultiAddr for node ", peer.NodeName, " is: [", output, "]")

log.Info("Adding node to the queue: [", peer.NodeName, "]")
go AddToQueue(peer)
Copy link
Member

Choose a reason for hiding this comment

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

It's not good that we are not doing error handling here

pkg/k8s/statefulsets.go Outdated Show resolved Hide resolved
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
Signed-off-by: Jose Ramon Mañes <[email protected]>
@Bidon15 Bidon15 requested a review from a team October 30, 2023 13:41
@tty47
Copy link
Author

tty47 commented Oct 30, 2023

we will review the pending points here:
https://github.com/celestiaorg/devops/issues/576

@tty47 tty47 closed this Oct 30, 2023
@tty47 tty47 reopened this Oct 30, 2023
@tty47 tty47 merged commit 6ae5bd1 into main Oct 30, 2023
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix Fixes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants