Simplify engine internals #3010
Merged
Conversation
This file contains 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
It's not used in any way as a struct. Signed-off-by: Roman Khimov <[email protected]>
That's where KISS and DRY get into a little conflict. My take is that KISS wins here: * it's five lines of code * yeah it's repetitive, but e.execIfNotBlocked() is repetitive as well with three lines of code minimum * it inherently requires lambdas * variables need to be defined and properly managed, some code looks a bit strange where we return (smth, error) and smth gets its value in a lambda, but error is the result of e.execIfNotBlocked() * in most cases it also leads to the need of additional "internal" functions * overall we get _less_ code after this patch * nothing hidden, clear execution flow Signed-off-by: Roman Khimov <[email protected]>
It's a simple alias since 6ad2624 and the only difference is an additional Hash() method that might as well be a part of shardWrapper method set. This also simplifies sortShardsByWeight, we don't need to hold the lock while sorting. Signed-off-by: Roman Khimov <[email protected]>
These _seriously_ complicate the code for zero reason. We have to: * create/pass some lambdas * mess with input/output variables * mess with iterator flow instead of shortcutting to exit * have fun with defers and named return variables Which leads to code bloat and increased cognitive load for developers. For what purpose? I have no idea, dropping them makes code much easier. Signed-off-by: Roman Khimov <[email protected]>
A lot of the same code, the only difference is apistatus.ObjectOutOfRange handling which doesn't hurt for Get and is very much relevant for GetRange. Signed-off-by: Roman Khimov <[email protected]>
roman-khimov
added
enhancement
roman-khimov
requested review from
carpawell and
cthulhu-rider
as code owners
carpawell
approved these changes
Nov 12, 2024
|
|
||
| // stop iterating over shards if SplitInfo structure is complete | ||
| return !outSI.GetLink().IsZero() && !outSI.GetLastPart().IsZero() | ||
| if !splitInfo.GetLink().IsZero() && !splitInfo.GetLastPart().IsZero() { | ||
| return logicerr.Wrap(objectSDK.NewSplitInfoError(splitInfo)) |
There was a problem hiding this comment.
That in fact is one of the things I wanted to discuss. It looks like split info collection we attempt to do here (with iteration-persistent splitInfo) is totally useless, we can return immediately upon seeing any split info because that's what we do ultimately anyway. But maybe I'm missing something.
There was a problem hiding this comment.
split information may or may not have a link object and a last part (it is theoretically ok to have a middle object only, be resynced, etc)
There was a problem hiding this comment.
Regarding the comment above --- having a complete split info is beneficial to the caller (even though it gets an error), so this if can't be removed even though lacking a proper info we will return whatever is available.
Regarding break/return --- both are valid, so it doesn't matter much, post-loop can be extended in which case return is better even though currently it duplicates error wrapping a bit.
|
|
||
| // stop iterating over shards if SplitInfo structure is complete | ||
| return !outSI.GetLink().IsZero() && !outSI.GetLastPart().IsZero() | ||
| if !splitInfo.GetLink().IsZero() && !splitInfo.GetLastPart().IsZero() { | ||
| return nil, logicerr.Wrap(objectSDK.NewSplitInfoError(splitInfo)) |
|
|
||
| // stop iterating over shards if SplitInfo structure is complete | ||
| return !outSI.GetLink().IsZero() && !outSI.GetLastPart().IsZero() | ||
| if !splitInfo.GetLink().IsZero() && !splitInfo.GetLastPart().IsZero() { | ||
| return nil, logicerr.Wrap(objectSDK.NewSplitInfoError(splitInfo)) |
cthulhu-rider
approved these changes
Nov 13, 2024
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.
This continues #3008 with internal refactoring that is mostly about making code simpler and dropping useless code.