Repository navigation
Core: Make metadata refresh retries configurable in JDBC catalog - #18539
Conversation
|
@pvary could you please have a look? |
|
Do we want to keep this as a JDBC only config, or do we want to have the same config for every other catalog as well? Based on this decision we either need to change the config name, or the we need to add implementation to the other catalogs. |
| this.fileIO = fileIO; | ||
| this.connections = dbConnPool; | ||
| this.catalogProperties = catalogProperties; | ||
| this.metadataRefreshMaxRetries = JdbcUtil.metadataRefreshMaxRetries(catalogProperties); |
There was a problem hiding this comment.
Shall we calculate this once for the catalog?
There was a problem hiding this comment.
Done. It's now parsed and validated once in JdbcCatalog.initialize() and passed to the operations classes, so a bad value fails at catalog startup. I kept the existing protected JdbcViewOperations constructor for compatibility.
| */ | ||
| public static final String METADATA_REFRESH_MAX_RETRIES = "metadata-refresh.max-retries"; | ||
|
|
||
| public static final int METADATA_REFRESH_MAX_RETRIES_DEFAULT = 20; |
There was a problem hiding this comment.
Do we want to use this default in the other places?
There was a problem hiding this comment.
With the property now JDBC-specific, the default lives in JdbcUtil. I left the 20 in the base operations classes alone so core doesn't reference a JDBC setting.
|
|
||
| @Test | ||
| public void testMetadataRefreshMaxRetries() { | ||
| JdbcCatalog jdbcCatalog = |
There was a problem hiding this comment.
Do we want to close these extra catalogs?
There was a problem hiding this comment.
Good catch, wrapped them in try-with-resources like the other tests here.
During a storage outage, loading a table or view retries the metadata read 20 times for up to 10 minutes, stalling clients. Add the jdbc.metadata-refresh-max-retries catalog property (default 20) to bound it.
cb204ea to
bd7c8c9
Compare
I'd keep it JDBC-only, following #3379, which added a Hive-specific key. Renamed it to |
| } | ||
|
|
||
| @Test | ||
| void metadataRefreshMaxRetries() { |
There was a problem hiding this comment.
For now, use the same pattern as the other test in the file.
We do:
- new test class - package private and package private test methods, and no test prefix
- old test class - stick to the pattern used there, and only change if there only a few existing tests
There was a problem hiding this comment.
Done, switched back to the public void testXxx pattern used in both files.
| } | ||
|
|
||
| @Test | ||
| void metadataRefreshMaxRetries() { |
|
@pvary thank you for the review! All comments are addressed now ✅ |
|
Merged to main. |
|
Thanks @pvary for the quick review and merge! |
2 participants
During a storage outage,
JdbcTableOperationsandJdbcViewOperationsretry reading the metadata file 20 times, for up to 10 minutes, before a table or view load fails. Clients stall waiting for these retries, and in a REST catalog server backed byJdbcCatalogeach stalled load also holds a request thread. This adds ajdbc.metadata-refresh-max-retriescatalog property, similar toiceberg.hive.metadata-refresh-max-retriesadded for Hive in #3379. The default stays at 20. Each retry is a full metadata read, so the total time also depends on the FileIO's own timeouts and retries, such ashttp-client.apache.socket-timeout-msands3.retry.num-retriesforS3FileIO.Test plan: refreshing a table or a view makes
max-retries + 1metadata reads; a negative value fails catalog initialization.