-
Notifications
You must be signed in to change notification settings - Fork 475
refactor(s3stream): refactor StreamReader Block object #2685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor(s3stream): refactor StreamReader Block object #2685
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors the StreamReader's Block object by extracting a new ExternalCacheBlock
class to handle external cache operations, separating concerns between internal block management and external cache interactions.
- Introduces a new
ExternalCacheBlock
class for handling external cache-specific operations - Updates method signatures and variable declarations to use
ExternalCacheBlock
where appropriate - Moves some logic from the existing
Block
class to the newExternalCacheBlock
class
data.release(); | ||
data = null; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loadCf field may be null when release() is called, which would cause a NullPointerException. Consider adding a null check before calling whenComplete.
}); | |
if (loadCf != null) { | |
loadCf.whenComplete((v, ex) -> { | |
if (data != null) { | |
data.release(); | |
data = null; | |
} | |
}); | |
} |
Copilot uses AI. Check for mistakes.
// the data block is first loaded or evict & reload | ||
externalBlock.loadCf = dataBlockCache.getBlock(getOptions, objectReader, index).thenAccept(newData -> { | ||
externalBlock.data = newData; | ||
handleDataBlockChange(newData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handleDataBlockChange method is called from ExternalCacheBlock but is defined in the Block class, which will cause a compilation error since ExternalCacheBlock doesn't have access to this method.
Copilot uses AI. Check for mistakes.
} | ||
released = true; | ||
loadCf.whenComplete((nil, ex) -> { | ||
private void handleDataBlockChange(DataBlock newData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handleDataBlockChange method references instance fields (readCompleted, data, freeListenerHandle) from the Block class, but it's being called from ExternalCacheBlock which doesn't have these fields. This will cause compilation errors.
Copilot uses AI. Check for mistakes.
No description provided.