feat(auto-salvage): v2 support #70
Conversation
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
2 times, most recently
from
10b92b6
to
7fbc273
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 77.44% 77.58% +0.14%
==========================================
Files 35 35
Lines 1920 1932 +12
==========================================
+ Hits 1487 1499 +12
Misses 317 317
Partials 116 116
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
2 times, most recently
from
e697ada
to
ee33d7b
Compare
utils/misc.go
Outdated
| sort.Slice(keys, func(i, j int) bool { | ||
| return reflect.ValueOf(keys[i]).String() < reflect.ValueOf(keys[j]).String() | ||
| }) | ||
| case reflect.Uint64: |
There was a problem hiding this comment.
Not a huge deal, but why not make this library method work for any type that CanUint() ?
There was a problem hiding this comment.
Thank you. I've made this more general. For the test cases, I am writing this for the v2 volume support auto salvage feature. We can extend with additional cases in future enhancements.
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
ee33d7b
to
86c4cfa
Compare
📝 WalkthroughWalkthroughThe changes introduce a new function Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
utils/misc_test.go (1)
247-300: LGTM! Well-structured test function with good coverage.The
TestSortKeysfunction is well-structured and covers a good range of scenarios, including edge cases. The test cases are clearly defined, and the assertions are appropriate.Consider the following improvements:
- Add a test case for a map with mixed key types to ensure the function handles this scenario correctly.
- For numeric key types (e.g., uint64), add an explicit check to verify the correct ordering of the sorted keys.
- Enhance the error messages in the assertions to provide more context. For example:
c.Assert(err, IsNil, Commentf("Expected no error for %s, but got: %v", testName, err)) c.Assert(result, DeepEquals, testCase.expected, Commentf("Unexpected result for %s. Got %v, expected %v", testName, result, testCase.expected))These improvements will make the tests more robust and easier to debug if they fail.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- utils/misc.go (2 hunks)
- utils/misc_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
utils/misc.go (1)
10-10: LGTM: Import statement added correctly.The
sortpackage import is necessary for the newSortKeysfunction and is placed correctly in alphabetical order.
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
86c4cfa
to
6818500
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- utils/misc.go (2 hunks)
- utils/misc_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
utils/misc.go (1)
125-154: 🛠️ Refactor suggestionConsider enhancing the flexibility and performance of the
SortKeysfunction.While the current implementation is a good start, there are several areas for improvement:
Limited type support: The function only handles string and uint64 key types. Consider extending this to support more types or allowing custom comparison functions.
Use of reflection: The current implementation uses reflection, which may impact performance for large maps. Consider using type switches or type constraints for better performance.
Error handling: The error for unsupported types is returned after creating and partially processing the slice. It would be more efficient to check the type earlier.
PR objectives: It's not clear how this function relates to the PR objectives of implementing v2 volume support for auto salvage. Consider adding a comment explaining its role in the larger feature implementation.
Here's a suggested refactor that addresses these points:
import ( "golang.org/x/exp/constraints" "sort" ) func SortKeys[K constraints.Ordered, V any](mapObj map[K]V) ([]K, error) { if mapObj == nil { return nil, fmt.Errorf("input object cannot be nil") } keys := make([]K, 0, len(mapObj)) for key := range mapObj { keys = append(keys, key) } sort.Slice(keys, func(i, j int) bool { return keys[i] < keys[j] }) return keys, nil }This version:
- Uses the
constraints.Orderedinterface to support all ordered types.- Avoids reflection for better performance.
- Simplifies the implementation by leveraging Go's type system.
To ensure this change doesn't break existing usage, please run the following command:
Would you like me to provide an implementation that allows for custom comparison functions, or to explain how this utility fits into the larger auto salvage feature?
✅ Verification successful
The review comment regarding the
SortKeysfunction is valid and can be implemented safely.
- The function is primarily used in
utils/misc_test.goand vendor packages, minimizing the risk of impacting the main codebase.- Enhancing flexibility and performance as suggested will improve utility without adverse effects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of SortKeys in the codebase rg "SortKeys\(" --type goLength of output: 746
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
6818500
to
c59189e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
utils/misc.go (1)
125-141: LGTM with suggestions for further improvements.The
SortKeysfunction is well-implemented and addresses the main objective of sorting map keys in ascending order. It uses generics withconstraints.Ordered, which provides good type safety and flexibility. The error handling for nil input is a good practice.However, there are a few potential improvements to consider:
- Performance: For large maps, using
sort.Slicemight not be the most efficient approach due to its use of reflection.- Customization: The function doesn't allow for custom sorting logic, which might be useful in some scenarios.
Consider the following enhancements:
- For better performance, you could use
sort.Sortwith a custom sorter implementation instead ofsort.Slice.- To allow for custom sorting logic, you could add an optional comparison function parameter.
Here's a potential signature that addresses these points:
func SortKeys[K constraints.Ordered, V any](mapObj map[K]V, less func(K, K) bool) ([]K, error)This would allow callers to provide their own sorting logic if needed, while still using the default comparison if no function is provided.
Would you like me to provide an example implementation of this more flexible approach?
utils/misc_test.go (2)
248-288: LGTM: Comprehensive test cases for SortKeys function.The
TestSortKeysfunction provides good coverage for various scenarios, including edge cases (nil and empty maps) and different key types (string and uint64). The use of generics allows for efficient testing of multiple key types.Consider adding a test case with a larger number of keys to ensure the sorting works correctly for bigger datasets.
296-308: LGTM: Well-implemented generic test runner.The
runTestSortKeysfunction effectively executes the test cases for different key types. It properly handles both error and success scenarios.Consider enhancing the error messages to provide more context:
c.Assert(err, IsNil, Commentf("Unexpected error in %v: %v", testName, err)) c.Assert(result, DeepEquals, tc.expected, Commentf("Unexpected result in %v. Got: %v, Expected: %v", testName, result, tc.expected))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- go.mod (1 hunks)
- utils/misc.go (2 hunks)
- utils/misc_test.go (2 hunks)
- vendor/golang.org/x/exp/LICENSE (1 hunks)
- vendor/golang.org/x/exp/PATENTS (1 hunks)
- vendor/golang.org/x/exp/constraints/constraints.go (1 hunks)
- vendor/modules.txt (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- vendor/golang.org/x/exp/LICENSE
🧰 Additional context used
🔇 Additional comments (7)
vendor/golang.org/x/exp/PATENTS (1)
1-22: Verify the necessity and implications of including this patent grant.The addition of the "Additional IP Rights Grant (Patents)" file from Google's Go project is a significant legal document. While it provides patent protection for users of the Go implementation, it's important to consider the following:
- Ensure that including this file aligns with your project's licensing strategy and doesn't conflict with existing licenses.
- Verify that your project actually uses code from
golang.org/x/expthat necessitates including this patent grant.- Be aware that this grant includes a termination clause in case of patent litigation, which could affect your project's users.
To confirm the necessity of this file, please run the following script:
This change seems tangential to the main objective of implementing a function to sort map keys. Please clarify how this relates to the PR's primary goal of adding v2 volume support for auto salvage.
go.mod (2)
Line range hint
5-5: Verify compatibility with the new Go toolchain version.The toolchain has been updated from go1.22.0 to go1.23.2, which is a significant version jump. This change may introduce new features or potential breaking changes.
Please ensure that:
- All dependencies are compatible with Go 1.23.2.
- The codebase has been tested thoroughly with the new toolchain version.
- CI/CD pipelines have been updated to use Go 1.23.2.
Run the following script to check for any compatibility issues:
16-16: Clarify the purpose of the new experimental dependency.A new experimental dependency has been added:
golang.org/x/exp v0.0.0-20230515195305-f3d0a9c9a5ccPlease provide information on:
- The specific features or functions from this package that are being used in the project.
- Why this experimental package was chosen over stable alternatives, if any exist.
- Any potential risks or limitations associated with using this experimental package.
Additionally, consider adding a comment in the code or documentation explaining the purpose of this dependency for future reference.
To verify the usage of this package, run the following script:
utils/misc_test.go (3)
6-6: LGTM: New import for constraints package.The addition of the
golang.org/x/exp/constraintsimport is appropriate for the use of theOrderedconstraint in the new generic types and functions.
290-294: LGTM: Well-defined generic test case type.The
testCaseSortKeystype is well-structured and uses appropriate generic constraints. It provides all necessary fields for comprehensive testing of theSortKeysfunction.
248-308: Great job on implementing comprehensive tests for SortKeys!The new test implementation for
SortKeysis well-structured, uses generics effectively, and covers various scenarios. It integrates seamlessly with the existing test suite and follows consistent coding patterns.Some minor suggestions for further improvement:
- Consider adding a test case with a larger number of keys.
- Enhance error messages in assertions to provide more context.
- Add a test case for unsupported key types to ensure proper error handling.
Overall, excellent work on expanding the test coverage!
vendor/modules.txt (1)
146-148: LGTM: Addition of golang.org/x/exp moduleThe addition of the
golang.org/x/expmodule, specifically theconstraintssubmodule, is appropriate for implementing generic sorting functions. This aligns well with the PR objective of sorting map keys.Let's verify the usage of this new module in the codebase:
✅ Verification successful
Verified: golang.org/x/exp/constraints Usage Confirmed
The usage of the
golang.org/x/exp/constraintsmodule has been confirmed in the following files:
utils/misc_test.go(lines 290, 296)utils/misc.go(line 126)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of golang.org/x/exp/constraints in the codebase # Test: Search for import statements using the new module echo "Checking for import statements:" rg -n 'import.*"golang.org/x/exp/constraints"' # Test: Search for usage of types from the constraints package (e.g., Ordered) echo "Checking for usage of constraints.Ordered:" rg -n 'constraints\.Ordered'Length of output: 551
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <[email protected]>
derekbit
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
c59189e
to
29da146
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
utils/misc_test.go (3)
6-6: Consider documenting the use of experimental package.The
golang.org/x/exp/constraintspackage is experimental. While it's the recommended way to implement generic constraints, it's worth documenting this dependency and potential future migration needs when the package becomes stable.
248-288: Consider adding more comprehensive test cases.The test coverage is good for basic scenarios, but consider adding these test cases:
- Maps with duplicate values but unique keys
- Maps with special characters or unicode strings
- Large maps to verify performance
- Maps with maximum uint64 values
Example:
"SortKeys(...): unicode strings": { inputMap: map[string]any{ "世界": "", "Hello": "", "你好": "", }, expected: []string{"Hello", "世界", "你好"}, }, "SortKeys(...): large numbers": { inputMap: map[uint64]any{ math.MaxUint64: "", math.MaxUint64 - 1: "", }, expected: []uint64{0, math.MaxUint64 - 1, math.MaxUint64}, },
296-308: Consider enhancing error messages for better debugging.While the test runner is well-structured, the error messages could be more descriptive. Consider:
-c.Assert(err, NotNil, Commentf("Expected error in %v", testName)) +c.Assert(err, NotNil, Commentf("Expected error for test case %v with input: %v", testName, tc.inputMap)) -c.Assert(err, IsNil, Commentf("Unexpected error in %v", testName)) +c.Assert(err, IsNil, Commentf("Unexpected error in %v: %v", testName, err)) -c.Assert(result, DeepEquals, tc.expected, Commentf("Unexpected result in %v", testName)) +c.Assert(result, DeepEquals, tc.expected, Commentf("Test case %v: expected %v but got %v", testName, tc.expected, result))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
go.modis excluded by!go.modvendor/golang.org/x/exp/LICENSEis excluded by!vendor/**vendor/golang.org/x/exp/PATENTSis excluded by!vendor/**vendor/golang.org/x/exp/constraints/constraints.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
- utils/misc.go (2 hunks)
- utils/misc_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/misc.go
🔇 Additional comments (1)
utils/misc_test.go (1)
290-294: LGTM! Well-structured test case type.The generic type definition with
constraints.Orderedconstraint is well-designed and provides good type safety.
Yes! It's just a general-purposed lib and is ready for merge. Thank you! |
Which issue(s) this PR fixes:
Issue longhorn/longhorn#8430
What this PR does / why we need it:
Implement a new function to sort the keys of a map in ascending order.
Special notes for your reviewer:
NoneAdditional documentation or context
None