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

log: Clean-up operation loop #2294

Merged
merged 9 commits into from
Feb 1, 2021
Merged

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Jan 25, 2021

This change simplifies log operation loop in preparation for a structural refactoring.
Review commit-by-commit.

#1640

@pav-kv pav-kv requested a review from a team as a code owner January 25, 2021 11:24
@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 25, 2021

PTAL. Commit-by-commit review is recommended.

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 25, 2021

Also, I recommend to "Hide whitespace changes" when reviewing this.

Copy link
Member

@AlCutter AlCutter left a comment

Choose a reason for hiding this comment

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

Nice commit-wise PR, thanks!

Copy link
Contributor

@Martin2112 Martin2112 left a comment

Choose a reason for hiding this comment

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

This seems to be functionally equivalent, or at least I didn't see any issues.

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 25, 2021

Travis is stuck, doesn't start any builds.

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 25, 2021

Added a mini-commit to trigger Travis again.

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 25, 2021

@Martin2112 @AlCutter I added another cosmetic commit, which eliminates the goto-like "break label" construction, by factoring out another method. You may want to review this last commit just in case.

@Martin2112
Copy link
Contributor

LGTM

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 28, 2021

Nice. New integration tests in Could Build pass. I will merge this after playing with CB a little more and ensuring it has all the necessary coverage.

A few lines above there is glog printing the number of processed
entries, and processing time. There is not much sense in aggregating the
number of processed entries across log IDs for glog purposes, this info
is better represented in already existing metrics.
This not only makes the code simpler, but prepares it for inverting
control in follow-up commits. Previously, workers invoked per-logID
functions, but after the refactoring each log will have a dedicated
worker.
@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #2294 (274664f) into master (2038950) will increase coverage by 0.46%.
The diff coverage is 81.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2294      +/-   ##
==========================================
+ Coverage   65.04%   65.51%   +0.46%     
==========================================
  Files         118      115       -3     
  Lines        8907     8816      -91     
==========================================
- Hits         5794     5776      -18     
+ Misses       2454     2383      -71     
+ Partials      659      657       -2     
Impacted Files Coverage Δ
client/admin.go 59.59% <ø> (ø)
client/log_verifier.go 73.91% <ø> (ø)
client/rpcflags/rpcflags.go 100.00% <ø> (ø)
merkle/logverifier/log_verifier.go 96.42% <ø> (ø)
merkle/sparse_merkle_tree.go 91.11% <ø> (ø)
quota/etcd/storage/quota_storage.go 82.41% <ø> (ø)
quota/mysqlqm/mysql_quota.go 58.62% <ø> (ø)
quota/mysqlqm/quota_provider.go 0.00% <ø> (ø)
server/interceptor/interceptor.go 79.39% <ø> (ø)
server/map_write_rpc_server.go 0.00% <0.00%> (ø)
... and 9 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 797941c...274664f. Read the comment docs.

@pav-kv pav-kv merged commit 75f1995 into google:master Feb 1, 2021
@pav-kv pav-kv deleted the log_operation_cleanup branch February 1, 2021 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants