-
Notifications
You must be signed in to change notification settings - Fork 472
feat: Making MRD Reader compatible to integrate with REadmanager #4193
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4193 +/- ##
==========================================
+ Coverage 83.19% 83.23% +0.04%
==========================================
Files 153 153
Lines 18716 18725 +9
==========================================
+ Hits 15570 15586 +16
+ Misses 2577 2572 -5
+ Partials 569 567 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // GCSReader defines an interface for reading data from a GCS object. | ||
| // This interface is intended for lower-level interactions with GCS readers. | ||
| type GCSReader interface { | ||
| // ReadAt reads data into the `Buffer` field of the provided `GCSReaderRequest`, |
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.
nit: Read ...
| // starting from the specified offset and ending at the specified end offset. | ||
| // It returns a `ReadResponse` indicating the number of bytes successfully read and any error encountered. | ||
| ReadAt(ctx context.Context, req *GCSReaderRequest) (ReadResponse, error) | ||
| Read(ctx context.Context, req *GCSReaderRequest) (ReadResponse, error) |
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.
Should we use a different name here like ReadFromOffset or ReadAtOffset (or something else) since we'll be providing an explicit offset to read from? (io.Reader's Read is a stateful read where the offset to read from is being maintained & not passed explicitly)
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
9 similar comments
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @charith87, @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
Renamed ReadAt() to Read method in GCSReader interface. This is to make mrd_reader.go compatible with both GCSReader and Reader interfaces.
Added ReadAt() method in mrd_reader.go. This helps in integrating mrdReader with read_manager.go directly.
Link to the issue in case of a bug fix.
Testing details
Any backward incompatible change? If so, please explain.