Add support for tracking Hadoop and GCS API level metrics at a thread… #1299
Conversation
… level GCS APIs which are called from a thread different the thre which called the Hadoop API is not captured. This will be fixed in a later change
|
/gcbrun |
gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsThreadLocalStatistics.java
Outdated
Show resolved
Hide resolved
| import org.apache.hadoop.fs.StorageStatistics; | ||
|
|
||
| class GhfsThreadLocalStatistics extends StorageStatistics { | ||
| static final String NAME = "GhfsThreadLocalStatistics"; |
There was a problem hiding this comment.
These are string with special meaning. The consumer framework (eg. spark) will use this name to get the statistics.
gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemIntegrationTest.java
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| void increment(GoogleCloudStorageStatistics op, long count) { |
There was a problem hiding this comment.
I am in double minds since we are not handling all the enum cases. Will change if we add values.
|
|
||
| class GhfsThreadLocalStatistics extends StorageStatistics { | ||
| static final String NAME = "GhfsThreadLocalStatistics"; | ||
| static final String HADOOP_API_COUNT = "hadoopApiCount"; |
There was a problem hiding this comment.
Enum would be better to represent this I think.
So ThreadLocalValue and corresponding strings stay together and not set different one by mistake.
There was a problem hiding this comment.
Same as above. The consumer framework is not aware of the enums and will have to use string to adhere to the interface.
gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemIntegrationTest.java
Outdated
Show resolved
Hide resolved
gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GhfsThreadLocalStatisticsTest.java
Show resolved
Hide resolved
gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsThreadLocalStatistics.java
Outdated
Show resolved
Hide resolved
|
|
||
| void increment(GhfsStatistic statistic, long count) { | ||
| if (statistic == GCS_CONNECTOR_TIME) { | ||
| this.hadoopApiTime.increment(count); |
There was a problem hiding this comment.
Sorry, I am not able to understand this, why are we incrementing hadoopApiTime for GCS_CONNECTOR_TIME?
Also, the below metric (2 lines below) is Count while here it is Time.
There was a problem hiding this comment.
GCS connector implements Hadoop FS APIs. So we are measuring the wall clock time taken by GcsConnectorHadoopAPIs.
Another way to look at this is: From the caller perspective this the "hadoopAPITime" and from the connector perspective it is the GCSConnectorTime. Also, from the caller perspective they need the metric to be generic since the connector could be for a different scheme.
|
/gcbrun |
… level
GCS APIs which are called from a thread different the thre which called the Hadoop API is not captured. This will be fixed in a later change