Reapply changes from reverted PR 3423 #4257
Conversation
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @KirillLykov. |
KirillLykov
force-pushed
the
klykov/reapply-3423
branch
from
78cadad to
268f2a7
Compare
can you edit the description to describe the pr in its own terms rather than in reference to a previous one? i don't want to have to go guess at the intended differences in 900 lines of changes |
| pub const LEADER_INFO_REFRESH_RATE_MS: u64 = 1000; | ||
|
|
||
| /// A struct responsible for holding up-to-date leader information | ||
| /// used for sending transactions. | ||
| pub struct CurrentLeaderInfo<T> |
There was a problem hiding this comment.
this and any other moved public interface needs to be reexported here, with deprecation annotations. even though the annotations don't actually work on re-exports, they give us documentation and something to grep for
There was a problem hiding this comment.
Done, added reexport of the public structures/const that have been moved.
There was a problem hiding this comment.
thanks! can you annotate with the rust attribute instead of a comment?
|
ideally this would be at least two PRs (i can't tell if more). first should exclusively re-organize the existing symbols in code, second(+) introduce new or changed logic. it is far too easy to miss logical changes in moved code |
This PR only moves these structures and makes a couple of small changes in the way config is passed. Hence, accumulates some cosmetic changes that simplify further logic modifications. It has been reviewed by other people, so I believe it is safe enough in terms of the logic and the only problem is API modification. This PR is already a first of the chain of 4 PRs that only make any difference all together, splitting them into smaller atomic peaces would make it harder to maintain this PR chain, rebasing etc. |
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
This is the same as PR#3423 but with minimized API changes in STS. In order to be able to use new client code in the `SendTransactionService`, I need to wrap network-related code with the new structure ConnectionCacheClient. To to that I also had to move some auxiliary code into separate files.
KirillLykov
force-pushed
the
klykov/reapply-3423
branch
from
268f2a7 to
69e7397
Compare
KirillLykov
force-pushed
the
klykov/reapply-3423
branch
from
69e7397 to
657ba60
Compare
the and here is the problem. i cannot confidently review such changes when made in the same commit and cannot approve changes that i cannot confidently review i say ideally these be separate PRs due to our being forced to use squash+merge and running into changes made like this during a crises is pure hell to sift through for relevancy while debugging |
This PR adds back changes from PR 3423 but this time with less API changes which allows to simplify backport to 2.1.
Problem
In order to be able to use new client code in the
SendTransactionService(see #3444), I need to wrap network-related code with the new structureConnectionCacheClient.Summary of Changes
To extract network code into a new structure, I had to move some auxiliary code into separate files.
Beside of that, changes are pretty minimal and I deliberately haven't tried to optimize existing code to keep changes minimal. It just does almost exactly the same as the old one.