HBASE-29402: Comprehensive key management for encryption at rest #7111
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Also committing WIP L1 cache implementation - Added JavaDoc
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
It took me several back and forth cycles to tell Cursor what I want and also make corrections to the code, so I asked it to summarize by generating a comprehensive prompt and the below is what I got. Saving it as a quick future reference. I will follow up with manaul improvements to this change and better test coverage. Here's a comprehensive prompt that captures all the incremental changes and corrections we made throughout this refactoring: --- **Comprehensive Refactoring Prompt for ManagedKeyDataCache and ManagedKeyAccessor** I want to refactor the key management system in HBase to improve performance, simplify the architecture, and eliminate redundant code. Here are the specific requirements: ## Initial Refactoring Goals 1. **Refactor ManagedKeyDataCache.getEntry() to use CacheLoader interface**: Remove redundant methods and delegate responsibility properly. Use Caffeine's `get()` method with a lambda function for loading to allow passing context parameters (`key_cust` and `keyNamespace`) directly. 2. **Refactor ManagedKeyAccessor to delegate all key retrieval logic to the cache**: Include dynamic provider access, simplifying the accessor to be a thin wrapper. 3. **Merge getActiveKeys method into getRandomEntry**: Make `cacheByNS` a separate Caffeine cache with its own config to handle only active keys efficiently. 4. **Simplify active keys cache structure**: Use a 2-level cache with a composite key (custodian + namespace) and a regular Map for keys, then change to a List for O(1) random selection. ## Cache Architecture Changes 5. **Change active keys cache from Map to List**: Change the value type in `activeKeysCache` from `Map<String, ManagedKeyData>` to `List<ManagedKeyData>` to make random selection lightweight (O(1) instead of O(n)). 6. **Remove redundant addEntry() calls**: Explicit `addEntry()` calls inside Caffeine callbacks are redundant since Caffeine automatically caches returned values. Remove these from `getEntry()`, `getRandomEntry()`, and `getActiveKeys()` methods. 7. **Update removeEntry to work with both caches**: The `removeEntry` method should remove from both the main cache and the active keys cache. 8. **Update getEntryCount to include both caches**: Make it lightweight and clearly indicate it's an estimate that may include double-counting. ## Active Keys Cache Behavior 9. **Remove automatic addition to active keys cache**: `addEntry()` should not add to the active keys cache to avoid suppressing bulk loading logic. During random key selection, include individual active entries from the main cache if missing from the active keys cache. 10. **Simplify getRandomEntry**: Use `cache.getIfPresent` instead of `cache.get` to avoid triggering loading callbacks when just checking what's already cached. 11. **Remove dynamic lookup from active keys cache callback**: The `isDynamicLookupEnabled()` check should not be used in the callback for `activeKeysCache` since the callback is for loading active keys from the cache, not for determining dynamic lookup. 12. **Ignore main cache entries in getRandomEntry**: The `getRandomEntry` method should only return entries from the active keys cache and ignore any entries that might exist in the main cache. ## Method Cleanup 13. **Remove getActiveKeys() method**: Since `activeKeysCache` already contains only active keys, the `getActiveKeys()` method is redundant. Remove it and simplify `ManagedKeyAccessor.getAnActiveKey()` method. 14. **Remove ManagedKeyAccessor entirely**: Once `ManagedKeyAccessor` has no meaningful logic left, remove it and replace usage with `ManagedKeyDataCache` directly. Update the `Server` interface to return `ManagedKeyDataCache` instead of `ManagedKeyAccessor`. 15. **Remove addEntry method**: If `addEntry` is not being used by any callers, remove it. If it's only used internally, replace with direct cache access. 16. **Remove unused methods**: Remove `getActiveEntriesFromCache()` method if it's not being used anywhere. ## Inheritance and Code Reuse 17. **Make KeyManagementBase the base class**: Make `ManagedKeyDataCache` extend `KeyManagementBase` so it can reuse the `getKeyProvider()` method and other common functionality like `getServer()`. ## Testing and Validation 18. **Update all test files**: Replace all usages of `ManagedKeyAccessor` with `ManagedKeyDataCache` in test files and mock classes. 19. **Fix test compilation issues**: Update test mocks to properly configure the server mock to return a valid Configuration object. 20. **Update test logic**: Modify tests to work with the new architecture where `getRandomEntry()` only looks at the active keys cache, not the main cache. ## Key Design Principles - **Performance**: Random key selection should be O(1) - **Simplicity**: Remove redundant layers and methods - **Correctness**: Maintain proper L1 → L2 → Dynamic Provider hierarchy - **Testability**: Ensure tests can still validate functionality - **Memory efficiency**: Use appropriate data structures for each use case ## Expected Final Architecture - **Main cache**: Caffeine cache for general key data with automatic loading - **Active keys cache**: Caffeine cache with List values for O(1) random selection - **No accessor layer**: Direct usage of `ManagedKeyDataCache` - **Inheritance**: `ManagedKeyDataCache` extends `KeyManagementBase` - **Clean separation**: Active keys cache only for active keys, main cache for all keys Please implement these changes incrementally, ensuring each step compiles and tests pass before proceeding to the next.
|
Closing temporarily to rework on caching layers. |
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.
JIRA: HBASE-29402
Please refer to parent JIRA: HBASE-29368 for details and a design doc.