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

Initialise New Scheduler #2050

Merged
merged 203 commits into from
Jan 25, 2023
Merged

Initialise New Scheduler #2050

merged 203 commits into from
Jan 25, 2023

Conversation

d80tb7
Copy link
Collaborator

@d80tb7 d80tb7 commented Jan 22, 2023

Code Necessary to Intialise new scheduler and start the scheduling cycle. Main changes in the this PR:

  • Scheduler now has subcommands (one for runnign the scheduler, another to migrate the db etc)
  • Implementation of QueueRepository that is just backed by the existing Redis code.
  • Scheduler has its config validated by https://github.com/go-playground/validator. If this works well we can do the same for other components
  • Full initialisation code for the scheduler (see schedulerapp for this)
  • Added viper hooks for the pulsar config enums so they are parsed as part of viper rather than in app code

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2023

Codecov Report

Base: 47.55% // Head: 47.87% // Increases project coverage by +0.32% 🎉

Coverage data is based on head (c508028) compared to base (fbb2227).
Patch coverage: 32.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2050      +/-   ##
==========================================
+ Coverage   47.55%   47.87%   +0.32%     
==========================================
  Files         212      218       +6     
  Lines       28809    29230     +421     
==========================================
+ Hits        13699    13995     +296     
- Misses      14122    14247     +125     
  Partials      988      988              
Flag Coverage Δ
airflow-operator 89.13% <ø> (ø)
armada-server 46.85% <32.66%> (+0.35%) ⬆️
python-client 93.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ernal/armada/repository/apimessages/conversions.go 82.06% <ø> (ø)
internal/armada/repository/event_store.go 0.00% <0.00%> (ø)
internal/armada/server/submit_to_log.go 0.00% <0.00%> (ø)
internal/common/config/redis.go 0.00% <0.00%> (ø)
internal/common/config/validation.go 0.00% <0.00%> (ø)
internal/common/database/functions.go 0.00% <0.00%> (ø)
internal/common/pulsarutils/eventsequence.go 74.13% <0.00%> (ø)
internal/common/pulsarutils/pulsarclient.go 100.00% <ø> (ø)
internal/eventingester/convert/conversions.go 60.46% <0.00%> (ø)
internal/scheduler/leader.go 93.58% <0.00%> (-6.42%) ⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@d80tb7 d80tb7 changed the title [WIP] Initialise New Scheduler Initialise New Scheduler Jan 23, 2023
cmd/scheduler/cmd/migrate_database.go Outdated Show resolved Hide resolved
cmd/scheduler/cmd/migrate_database.go Outdated Show resolved Hide resolved
if err != nil {
return errors.Wrapf(err, "Failed to migrate scheduler database")
}
taken := time.Now().Sub(start)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use time.Since instead.

config/scheduler/config.yaml Show resolved Hide resolved
"github.com/armadaproject/armada/internal/common/armadaerrors"
)

var CustomHooks = []viper.DecoderConfigOption{
Copy link
Contributor

Choose a reason for hiding this comment

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

Validation! So nice.

internal/scheduler/schedulerapp.go Outdated Show resolved Hide resolved
internal/scheduler/schedulerapp.go Outdated Show resolved Hide resolved
internal/scheduler/schedulerapp.go Show resolved Hide resolved
defer grpcServer.GracefulStop()
lis, err := net.Listen("tcp", fmt.Sprintf(":%d", config.Grpc.Port))
if err != nil {
return errors.WithMessage(err, "Error setting up grpc server")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return errors.WithMessage(err, "Error setting up grpc server")
return errors.WithMessage(err, "failed to setup grpc server")

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. But ut's recommended to start error messages with lower-case letters since thet're often strung together like "message 1: message 2` etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for other parts of this file.

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

}
return NewKubernetesLeaderController(LeaderConfig{}, clientSet.CoordinationV1()), nil
default:
return nil, errors.New(fmt.Sprintf("%s is not a value leader mode", config.Mode))
Copy link
Contributor

Choose a reason for hiding this comment

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

use errors.Errorf.

@d80tb7 d80tb7 merged commit 6b27ffe into master Jan 25, 2023
@d80tb7 d80tb7 deleted the f/chrisma/queue-repository branch January 25, 2023 10: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.

3 participants