grpc: backport block prunning fixes (#1271) to v0.38.x-experimental #1473
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* state: Invert logic of whether retain heights are set to be more readable Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor findMinRetainHeight logic for clarity and to respect config Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename findMinRetainHeight to findMinBlockRetainHeight for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename Pruner.SetApplicationRetainHeight to SetApplicationBlockRetainHeight for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename pruner SetCompanionRetainHeight to SetCompanionBlockRetainHeight for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable name Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - rename local method name for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - remove redundant condition Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - use helper instead of redefining logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor Pruner.SetApplicationBlockRetainHeight logic The application retain height should be set regardless of what the companion retain height is. Pruning should take place based on whichever of the two values is lower. There is no need to consider the companion retain height in this logic. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor Pruner.SetCompanionBlockRetainHeight logic Similar refactor to that done for Pruner.SetApplicationBlockRetainHeight. There is no need to consider the application retain height during this function call. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor Pruner.SetABCIResRetainHeight logic Aligns the logic of this method similar to that in the other retain height setter methods. Also fixes a logic bug where setting the retain height would skip updating the metrics. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Simplify pruner logic assuming retain heights are always set We can simplify the pruner logic if we assume that the initial retain heights are always set on node startup. Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Tell pruner whether companion is enabled Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Expand error message detail Signed-off-by: Thane Thomson <connect@thanethomson.com> * state+store: Align tests with new logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * rpc/grpc: Return trace IDs in error responses from pruning service Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Refactor/expand pruning service initialization logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Minor cosmetic tweaks to gRPC tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Enable data companion pruning Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Refactor - extract pruner creation as function Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Only enable companion-based pruning on full nodes and validators Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Only start ABCI results pruning routine when data companion is enabled Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Fix TestMinRetainHeight logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Fix companion retain height initialization logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Expand testing framework Allows explicit control over whether or not pruning should take a data companion into account for full nodes or validators in our E2E framework. This allows greater flexibility in the E2E framework to test nodes both with and without data companion-related functionality. Expands our standard CI manifest to expect data companions attached to two of the nodes. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename background routines Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Apply change from code review Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com>
This was referenced Oct 11, 2023
jmalicevic
approved these changes
Oct 11, 2023
lasarojc
pushed a commit
that referenced
this pull request
Nov 13, 2023
* state: Invert logic of whether retain heights are set to be more readable * state: Refactor findMinRetainHeight logic for clarity and to respect config * state: Rename findMinRetainHeight to findMinBlockRetainHeight for clarity * state: Rename Pruner.SetApplicationRetainHeight to SetApplicationBlockRetainHeight for clarity * state: Rename pruner SetCompanionRetainHeight to SetCompanionBlockRetainHeight for clarity * state: Refactor - shorten local variable name * state: Refactor - rename local method name for clarity * state: Refactor - remove redundant condition * state: Refactor - use helper instead of redefining logic * state: Refactor - shorten local variable names * state: Refactor - shorten local variable names * state: Refactor Pruner.SetApplicationBlockRetainHeight logic The application retain height should be set regardless of what the companion retain height is. Pruning should take place based on whichever of the two values is lower. There is no need to consider the companion retain height in this logic. * state: Refactor Pruner.SetCompanionBlockRetainHeight logic Similar refactor to that done for Pruner.SetApplicationBlockRetainHeight. There is no need to consider the application retain height during this function call. * state: Refactor Pruner.SetABCIResRetainHeight logic Aligns the logic of this method similar to that in the other retain height setter methods. Also fixes a logic bug where setting the retain height would skip updating the metrics. * state: Refactor - shorten local variable names * state: Simplify pruner logic assuming retain heights are always set We can simplify the pruner logic if we assume that the initial retain heights are always set on node startup. * node: Tell pruner whether companion is enabled * state: Expand error message detail * state+store: Align tests with new logic * state: Refactor - shorten local variable names * rpc/grpc: Return trace IDs in error responses from pruning service * node: Refactor/expand pruning service initialization logic * test/e2e: Minor cosmetic tweaks to gRPC tests * test/e2e: Enable data companion pruning * node: Refactor - extract pruner creation as function * test/e2e: Only enable companion-based pruning on full nodes and validators * state: Only start ABCI results pruning routine when data companion is enabled * state: Fix TestMinRetainHeight logic * node: Fix companion retain height initialization logic * test/e2e: Expand testing framework Allows explicit control over whether or not pruning should take a data companion into account for full nodes or validators in our E2E framework. This allows greater flexibility in the E2E framework to test nodes both with and without data companion-related functionality. Expands our standard CI manifest to expect data companions attached to two of the nodes. * state: Rename background routines * node: Apply change from code review --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partially addresses #1419:
This is part of the block pruning mechanism, it was not included in #1422 because it depends on #1471.
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments