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

Implement WAL static analysis to ensure that new entries will be properly annotated #13490

Merged
merged 6 commits into from
Feb 1, 2022

Conversation

serathius
Copy link
Member

@serathius serathius force-pushed the wal-static-analysis branch 5 times, most recently from 4afafd1 to f2305d4 Compare November 22, 2021 14:48
@serathius serathius mentioned this pull request Nov 22, 2021
13 tasks
@serathius serathius force-pushed the wal-static-analysis branch 2 times, most recently from 5021629 to a3150d8 Compare December 2, 2021 15:36
tools/proto-annotations/main.go Outdated Show resolved Hide resolved
server/storage/wal/version.go Show resolved Hide resolved
if valueOpts != nil {
maxVer = maxVersion(maxVer, etcdVersionFromOptionsString(valueOpts.String()))
ver, _ := etcdVersionFromOptionsString(valueOpts.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we are not parsing these options too frequently (e.g. on each interpreted data enum).
Alternatively we could build a map(s):

  • field-> version
  • enum -> version
    The way we build the version file... and during the data processing just keep performing lookups in the map.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we will definitely need this for version API calls as they will be latency sensitive, however I would skip this optimization of static analysis.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we afraid that start time of etcd server that will keep processing WAL log (ReadWALVersion) will get significantly slower. Have you tested it with realistic: O(64MB) Wal files ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't verify WAL during server start, we do it during downgrade and after we created a snapshot.

@serathius serathius force-pushed the wal-static-analysis branch from 76bdd0c to 7104b7a Compare January 26, 2022 14:50
@serathius
Copy link
Member Author

ping @ptabor

@serathius serathius force-pushed the wal-static-analysis branch from 7104b7a to f4187b4 Compare January 28, 2022 10:41
scripts/update_proto_annotations.sh Show resolved Hide resolved
tools/proto-annotations/cmd/etcd_version.go Outdated Show resolved Hide resolved
if valueOpts != nil {
maxVer = maxVersion(maxVer, etcdVersionFromOptionsString(valueOpts.String()))
ver, _ := etcdVersionFromOptionsString(valueOpts.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we afraid that start time of etcd server that will keep processing WAL log (ReadWALVersion) will get significantly slower. Have you tested it with realistic: O(64MB) Wal files ?

@serathius
Copy link
Member Author

@ptabor PTAL

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@682b867). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 846b339 differs from pull request most recent head 0c67c5c. Consider uploading reports for the commit 0c67c5c to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main   #13490   +/-   ##
=======================================
  Coverage        ?   72.68%           
=======================================
  Files           ?      472           
  Lines           ?    39361           
  Branches        ?        0           
=======================================
  Hits            ?    28610           
  Misses          ?     8861           
  Partials        ?     1890           
Flag Coverage Δ
all 72.68% <0.00%> (?)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 682b867...0c67c5c. Read the comment docs.

@ptabor
Copy link
Contributor

ptabor commented Jan 28, 2022

Thank you !

@serathius serathius merged commit ec8d880 into etcd-io:main Feb 1, 2022
@serathius serathius deleted the wal-static-analysis branch June 15, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants