GCP: Add metrics tracking for GCSFileIO #4267
Conversation
|
@danielcweeks Can you please review when you get time. If the implementation looks okay then I can go ahead with the other FileIOs. |
| GCSOutputStream stream = new GCSOutputStream(storage, randomBlobId(), properties, MetricsContext | ||
| .nullMetrics()); |
There was a problem hiding this comment.
Nit: Avoid breaking up a chained call as an argument across two lines. In this case I would move all arguments to the next line.
But having MetricsContext and then .nullMetrics() on the next line is confusing.
There was a problem hiding this comment.
Thanks, I have made the changes.
|
Hey @rajarshisarkar This is great. I'd actually point out there is some parallel work to make the initialization dynamic in #4254 and that looks like it's about to go in. You might want to take a quick peek and see if we can align with that approach. |
|
Thanks for having a look, @danielcweeks. Sure, I shall rebase once #4254 is merged to master. |
| public static GCSInputFile fromLocation(String location, Storage storage, GCPProperties gcpProperties) { | ||
| return new GCSInputFile(storage, BlobId.fromGsUtilUri(location), gcpProperties); | ||
|
|
||
| public static GCSInputFile fromLocation(String location, Storage storage, |
There was a problem hiding this comment.
Should this be public? Since it is a public constructor in a public class, we should deprecate the old constructor instead of directly removing it. If we want to avoid needing to deprecate in the future, then I think we should consider making this class and constructor package-private.
There was a problem hiding this comment.
Thanks, for the feedback. I have made the class and method package-private.
|
Most of this looks good. Thanks, @rajarshisarkar! |
|
Thanks, for the review @kbendick @danielcweeks @rdblue! Should we consider merging this PR (or) wait for #4254 to be merged to master? |
|
I'm going to merge this since there's still ongoing discussion about #4254. Thanks @rajarshisarkar! |
This PR adds metrics tracking for
GCSFileIO. This would help reporting IO metrics via the HadoopFileSystem.Statistics.