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

adding context timeouts for management queries based on reload interval #390

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rasamala83
Copy link
Collaborator

No description provided.

lib/racmaint.go Outdated
return
case <-timeTicker.C:
//Periodic data loading
racMaint(&ctx, shard, db, racSQL, cmdLineModuleName, prev, waitTime/2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let us use a configurable query timeout for maintenance queries. Here, the wait time could vary across pools.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

lib/racmaint.go Outdated
//First time data loading
racMaint(&ctx, shard, db, racSQL, cmdLineModuleName, prev, waitTime/2)

timeTicker := time.NewTicker(waitTime)
Copy link
Collaborator

Choose a reason for hiding this comment

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

defer timeTicker.Stop() to release resources.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed timeTicker.Stop() and reset

case <-ctx.Done():
logger.GetLogger().Log(logger.Alert, "Application main context has been closed, so exiting from racmaint data reload.")
return
case <-timeTicker.C:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with this but why do we need to introduce the timeTicker? I feel the existing code is simpler.

@@ -300,7 +305,7 @@ func InitShardingCfg() error {
}
db, err = openDb(shard)
if err == nil {
err = loadMap(ctx, db)
err = loadMap(&ctx, db, reloadInterval/2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. Let us use a configurable query timeout for maintenance queries

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}
go func() {
reloadTimer := time.NewTimer(reloadInterval) //Periodic reload timer
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be replaced with NewTicker as the timer fires just once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added reset and clean up options for timer

if err == nil {
err = loadMap(ctx, db)
select {
case <-ctx.Done():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let us also add timeout-related tests for validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added tests.

@venkatsridhar95
Copy link
Collaborator

@rasamala83 Converting the PR to draft as it is work in progress.

@venkatsridhar95 venkatsridhar95 marked this pull request as draft May 15, 2024 21:06
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.

3 participants