Track Glue API calls that were untracked

[homar]

Description

One call to Glue was not being tracked.

Rename GlueMetastoreStats.renameTable to GlueMetastoreStats.updateTable.

General information

Is this change a fix, improvement, new feature, refactoring, or other?

a fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

a connector

How would you describe this change to a non-technical end user or system administrator?

It will allow to more precisely track calls to external api - Glue

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`5678`)

[findepi]

are you positive this is the only untracked one?

[findepi]

the stats should be called "updateTable"

[findepi]

cc @losipiuk @alexjo2144

[homar]

are you positive this is the only untracked one?

I looked through other usages of glueClient and didn't spot anything else.

[findepi]

What about getAllDatabases and other places where we call getPaginatedResults?
or

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

Lines 429 to 433 in 262c6ec

	partitionUpdateRequestsFutures.add(glueClient.batchUpdatePartitionAsync(new BatchUpdatePartitionRequest()
	.withCatalogId(catalogId)
	.withDatabaseName(table.getDatabaseName())
	.withTableName(table.getTableName())
	.withEntries(partitionUpdateRequestsPartition)));

?

[homar]

What about getAllDatabases and other places where we call getPaginatedResults? or

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

Lines 429 to 433 in 262c6ec

	partitionUpdateRequestsFutures.add(glueClient.batchUpdatePartitionAsync(new BatchUpdatePartitionRequest()
	.withCatalogId(catalogId)
	.withDatabaseName(table.getDatabaseName())
	.withTableName(table.getTableName())
	.withEntries(partitionUpdateRequestsPartition)));

?

getPaginatedResults is covered everywhere, just look 1 or 2 lines above

Regarding the one you pointed out, maybe I am wrong but I thought there is no good way to measure this as this is an async call so we pretty much will measure only the time of the invocation of async which is not very informative

[losipiuk]

that was -> that were

[losipiuk]

LGTM - commit message may be a bit more accurate as you are also changing counter name

[losipiuk]

What about getAllDatabases and other places where we call getPaginatedResults? or

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

Lines 429 to 433 in 262c6ec

	partitionUpdateRequestsFutures.add(glueClient.batchUpdatePartitionAsync(new BatchUpdatePartitionRequest()
	.withCatalogId(catalogId)
	.withDatabaseName(table.getDatabaseName())
	.withTableName(table.getTableName())
	.withEntries(partitionUpdateRequestsPartition)));

?

getPaginatedResults is covered everywhere, just look 1 or 2 lines above

Regarding the one you pointed out, maybe I am wrong but I thought there is no good way to measure this as this is an async call so we pretty much will measure only the time of the invocation of async which is not very informative

You may want to add a callback on the returned future to bump statistic.
May be a follwoup.

[alexjo2144]

Not strictly necessary but a possible improvement from what we have. Would it be better if getPaginatedResults took a GlueMetastoreApiStats as a parameter and tracked individual API calls rather than wrapping the whole method with one?

Seems like that might be a more helpful way of tracking those stats to me.

[losipiuk]

there is more *async calls than just this one. We should address all together.

[losipiuk]

replace with single method

void recordCall(executionTime, boolean failure)

[losipiuk]

Not strictly necessary but a possible improvement from what we have. Would it be better if getPaginatedResults took a GlueMetastoreApiStats as a parameter and tracked individual API calls rather than wrapping the whole method with one?

Seems like that might be a more helpful way of tracking those stats to me.

Agreed. Currently lots of calls is still untracked.

[findepi]

If you retained the variable, the diff would be readable.

(btw the IDE asks me to inline such variables, but i acutally find it useful during debugging)

[findepi]

requireNonNull

[findepi]

same, no idea what changed

[losipiuk]

You should use different GlueMetastoreApiStats instance for different places when you use StatsRecordingAsyncHandler. Not stats.getBatchUpdatePartition() all the time.
Change parametrization of StatsRecordingAsyncHandler to use GlueMetastoreApiStats instead of GlueMetastoreStats

[homar]

yes definitely, I forgot to change that, thanks for catching this

[losipiuk]

use List

And keep variable name

[losipiuk]

Also add a followup commit which just gets rid of variables.

[losipiuk]

I think we should merge stats couneters for getting views and tables.

[losipiuk]

Can you also rename dropDatabase stats counter tdeleteDatabase.

(you can put all the renames in single commit)

[losipiuk]

Same for renameDatabase -> updateDatabase

[losipiuk]

And dropTable -> deleteTable

[losipiuk]

batchGetPartitionAsync is not covered with stats from what I see.

[losipiuk]

dropPartition -> deletePartition

[findepi]

skimmed, lgtm

[findepi]

This belongs to previous commit, where the last usage was removed.

[alexjo2144]

Seems like this belongs in GlueMetastoreApiStats

[losipiuk]

Let's change it as a followup :)