Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

[Feature] Adding new flag which enables custom block time value #384

Merged
merged 21 commits into from
Jan 27, 2022
Merged

[Feature] Adding new flag which enables custom block time value #384

merged 21 commits into from
Jan 27, 2022

Conversation

ZeljkoBenovic
Copy link
Contributor

Description

This PR introduces new flag --block-time which enables the operator to define a custom block time value during server configuration and startup.
This flag accepts values expressed in milliseconds and by default it is set to 2000 (2s).

Changed log output regarding block generation time to generation_time_in_miliseconds which now shows the value in milliseconds generation_time_in_miliseconds=2000

Fixed polygon_consensus_block_interval Prometheus metric to correctly show block generation time.
IMPORTANT: This metric is changed to Gauge, as Histogram did not present this metric properly.

Fixed polygon_consensus_rounds Prometheus metric to correctly show round number to correspond to the log outputs.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have tested this code
  • I have updated the README and other relevant documents (guides...)
  • I have added sufficient documentation both in code, as well as in the READMEs

@github-actions
Copy link

github-actions bot commented Jan 25, 2022 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@ZeljkoBenovic
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@ZeljkoBenovic ZeljkoBenovic marked this pull request as ready for review January 25, 2022 14:01
@ZeljkoBenovic ZeljkoBenovic self-assigned this Jan 25, 2022
zivkovicmilos
zivkovicmilos approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good, left a few minor comments 💯

@zivkovicmilos
Copy link
Contributor

@Kourin1996 @lazartravica

We should discuss if we want this block-time value to be unbounded - meaning people can set it to 0

@zivkovicmilos zivkovicmilos added the feature New update to Polygon Edge label Jan 25, 2022
@lazartravica
Copy link
Contributor

I don't want to constrain people not to set it to 0 if they want to, let's keep it completely unbounded right now so that we may test however we wish, and later we can add sane limitations

cc @zivkovicmilos @Kourin1996

lazartravica
lazartravica suggested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@lazartravica lazartravica left a comment

Choose a reason for hiding this comment

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

Just add small comments, and that's it :)

Thank you for fixing the metrics 🙏

command/helper/config.go Outdated Show resolved Hide resolved
command/helper/config.go Outdated Show resolved Hide resolved
consensus/consensus.go Outdated Show resolved Hide resolved
Kourin1996
Kourin1996 suggested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@Kourin1996 Kourin1996 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've left nitpick comment. Please check it. Thank you

command/server/server_command.go Outdated Show resolved Hide resolved
consensus/ibft/ibft.go Outdated Show resolved Hide resolved
consensus/ibft/ibft.go Outdated Show resolved Hide resolved
consensus/ibft/ibft.go Outdated Show resolved Hide resolved
consensus/ibft/ibft.go Outdated Show resolved Hide resolved
@ZeljkoBenovic
* Added comments to indicate miliseconds
0c6831c 
* Changed desctiption default value
* Changed time.UnixMili to time.Unix for go backward compatibility
github-actions bot added a commit that referenced this pull request Jan 26, 2022
@github-actions
@ZeljkoBenovic has signed the CLA from Pull Request #384
ff851d2
Kourin1996
Kourin1996 approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@Kourin1996 Kourin1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@ZeljkoBenovic ZeljkoBenovic mentioned this pull request Jan 27, 2022
Merged
@ZeljkoBenovic ZeljkoBenovic merged commit a5313a9 into develop Jan 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 27, 2022
@zivkovicmilos zivkovicmilos deleted the feature/configurable_block_time branch January 27, 2022 14:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dbrajovic dbrajovic dbrajovic approved these changes

@Kourin1996 Kourin1996 Kourin1996 approved these changes

@zivkovicmilos zivkovicmilos zivkovicmilos approved these changes

@lazartravica lazartravica lazartravica approved these changes

@brkomir brkomir Awaiting requested review from brkomir

@munna0908 munna0908 Awaiting requested review from munna0908

@0xAleksaOpacic 0xAleksaOpacic Awaiting requested review from 0xAleksaOpacic

Assignees

@ZeljkoBenovic ZeljkoBenovic

Labels
feature New update to Polygon Edge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ZeljkoBenovic @zivkovicmilos @lazartravica @Kourin1996 @dbrajovic