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

New slot ticker #1149

Merged
merged 19 commits into from
Oct 10, 2023
Merged

New slot ticker #1149

merged 19 commits into from
Oct 10, 2023

Conversation

MatusKysel
Copy link
Contributor

@MatusKysel MatusKysel commented Sep 20, 2023

New feed-less, gorountine-less slot ticker that is based on golang Timer

@MatusKysel MatusKysel force-pushed the new-slot-ticker branch 2 times, most recently from d506b83 to ed37f67 Compare September 21, 2023 07:04
@MatusKysel MatusKysel force-pushed the new-slot-ticker branch 2 times, most recently from 66b02e8 to 5ff3421 Compare September 21, 2023 20:26
@MatusKysel MatusKysel changed the title WIP: New slot ticker New slot ticker Sep 22, 2023
@MatusKysel MatusKysel marked this pull request as ready for review September 22, 2023 06:20
operator/slot_ticker/slotticker_test.go Outdated Show resolved Hide resolved
beacon/goclient/goclient.go Outdated Show resolved Hide resolved
operator/fee_recipient/controller_test.go Show resolved Hide resolved
operator/slot_ticker/slotticker.go Outdated Show resolved Hide resolved
nkryuchkov
nkryuchkov previously approved these changes Sep 25, 2023
Copy link
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Great work! Approving. Just one minor question

beacon/goclient/goclient.go Outdated Show resolved Hide resolved
nkryuchkov
nkryuchkov previously approved these changes Sep 25, 2023
operator/duties/scheduler_test.go Outdated Show resolved Hide resolved
operator/duties/scheduler_test.go Show resolved Hide resolved
Copy link
Contributor

@moshe-blox moshe-blox left a comment

Choose a reason for hiding this comment

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

I really liked the slot skipping test 👌

Left only 1 question.

operator/slotticker/slotticker_test.go Outdated Show resolved Hide resolved
nkryuchkov
nkryuchkov previously approved these changes Oct 4, 2023
y0sher
y0sher previously approved these changes Oct 5, 2023
@moshe-blox moshe-blox dismissed stale reviews from y0sher and nkryuchkov via e862fea October 9, 2023 11:30
@MatusKysel MatusKysel force-pushed the new-slot-ticker branch 2 times, most recently from 7150604 to faa6416 Compare October 9, 2023 13:20
"github.com/bloxapp/ssv/protocol/v2/types"
)

//go:generate mockgen -package=mocks -destination=./mocks/scheduler.go -source=./scheduler.go

var slotDelayHistogram = promauto.NewHistogram(prometheus.HistogramOpts{
Name: "slot_ticker_delay_milliseconds",
Copy link
Contributor

@nkryuchkov nkryuchkov Oct 10, 2023

Choose a reason for hiding this comment

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

According to Prometheus naming guidelines:

...must have a single unit (i.e. do not mix seconds with milliseconds, or seconds with bytes).
...should use base units (e.g. seconds, bytes, meters - not milliseconds, megabytes, kilometers). See below for a list of base units.

Also, other metrics in the project use the _seconds suffix, so we might need to consider making them consistent. On the other hand, I don't foresee any issues if we don't change it. So this is rather a suggestion, not a change request.

@MatusKysel MatusKysel merged commit 090b237 into stage Oct 10, 2023
5 checks passed
@MatusKysel MatusKysel deleted the new-slot-ticker branch October 10, 2023 18:30
y0sher added a commit that referenced this pull request Oct 15, 2023
* Message validation (#1066)
---------

Co-authored-by: moshe-blox <moshe@blox.io>
Co-authored-by: moshe-blox <89339422+moshe-blox@users.noreply.github.com>

* e2e tests for eth execution layer package (#1143)

* New slot ticker (#1149)

---------

Co-authored-by: Matus Kysel <matus@Matuss-MacBook-Air.local>
Co-authored-by: moshe-blox <moshe@blox.io>

---------

Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
Co-authored-by: moshe-blox <moshe@blox.io>
Co-authored-by: moshe-blox <89339422+moshe-blox@users.noreply.github.com>
Co-authored-by: Anton Korpusenko <antokorp@gmail.com>
Co-authored-by: Matus Kysel <MatusKysel@users.noreply.github.com>
Co-authored-by: Matus Kysel <matus@Matuss-MacBook-Air.local>
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.

4 participants