Split executeRange function

[serathius]

/cc @ahrtr @fuweid @siyuanfoundation

[serathius]

/retest

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.81%. Comparing base (ef21176) to head (c3147b5).
Report is 21 commits behind head on main.

❗ Current head c3147b5 differs from pull request most recent head 28546fa

Please upload reports for the commit 28546fa to get more accurate results.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdserver/txn/txn.go	93.92% <100.00%> (-0.76%)	⬇️

... and 30 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19808      +/-   ##
==========================================
- Coverage   68.87%   68.81%   -0.06%     
==========================================
  Files         424      421       -3     
  Lines       35876    35872       -4     
==========================================
- Hits        24710    24687      -23     
- Misses       9739     9761      +22     
+ Partials     1427     1424       -3     

---

Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[k8s-ci-robot]

@serathius: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-coverage-report	28546fa	link	true	/test pull-etcd-coverage-report

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[fuweid]

LGTM on green

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fuweid, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [fuweid,serathius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ahrtr]

I'm not a big fan of this refactoring style. Breaking one method into many small ones often hurts readability, as it forces readers to jump around the codebase more.

This small refactor seems harmless, but another similar bad example is the refactoring on NewServer , which splits one big (but easy to understand) method into too many small methods/functions. Based on my years of comparing the source code between 3.5 and 3.6, the NewServer of 3.5 is much much easier to understand than 3.6.

Also any change on the core applying workflow should allow more time to review!

[serathius]

Breaking one method into many small ones often hurts readability, as it forces readers to jump around the codebase more.

The goal isn't just more functions, but better abstraction. Well-defined, descriptively named sub-functions can clarify higher-level logic, allowing readers to understand the main flow without needing to read every underlying implementation detail immediately. Poor abstractions can indeed cause the issue you described.

Regarding NewServer (/pull/13230): while a large constructor might seem familiar, refactoring it, even if the current abstractions aren't perfect, helped uncover issues like circular dependencies and improved testability. This paves the way for further simplification. The executeRange refactor is a much smaller, more contained case, making it easier to get the abstractions right.

For executeRange, splitting the ~100-line function into clear steps (calculate limit, fetch, filter, sort, prepare response) aims to make the overall process more evident at a glance. Each sub-function has a single, clear responsibility.

[serathius]

Also any change on the core applying workflow should allow more time to review!

Code changes were reviewed by @fuweid, one of the benefit of simple refactors like this is that they don't change any business logic thus are much safer and can be reviewed faster.

[fuweid]

Today I reviewed several pull requests in the core package. Many of them focus on cleanup—reducing arguments, splitting logic into different files, and so on. From a reviewer’s perspective, these changes seem relatively safe since we have tests to cover them.

Whether to keep a function large or split it into smaller ones is always a tradeoff and depends on the context. A large function might span more than one screen, which still requires scrolling or jumping. On the other hand, small, well-named functions can help by clearly indicating each step. For me, this executeRange function has five distinct stages:

• Check limitations
• Perform RangeKeys
• Filter results
• Sort if needed
• Return the final result

The flow is clear and understandable. If a larger function lacks this kind of clear structure, then breaking it into smaller functions might not help readability.

Anyway, in terms of code style, I believe keeping the exposed API stable is more important than internal refactoring. Internal code can always be adapted later to support new features when needed.

:)

[ahrtr]

For executeRange, splitting the ~100-line function into clear steps (calculate limit, fetch, filter, sort, prepare response) aims to make the overall process more evident at a glance. Each sub-function has a single, clear responsibility.

It's a little over-engineering. I am not against encapsulation or abstraction, we should avoid too small granularity encapsulation.

This PR isn't that obvious, but we should try to avoid such trend.

[siyuanfoundation]

I agree we should avoid too small granularity encapsulation. A good encapsulation should be functionally complete and independent with descriptive names, which is well represented in this PR.

Breaking down a large function helps people, especially new contributors to understand the purposes and boundaries of different sections. Overall I think it improves readability.

[siyuanfoundation]

/lgtm