Use metastore locking around read-modify-write operations for transaction commit by findepi · Pull Request #9584 · trinodb/trino

findepi · 2021-10-11T10:36:27Z

findepi · 2021-10-11T10:39:15Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/HiveTableOperations.java

This breaks abstractions, since the class was supposed to be dependent on HiveMetastore only.
This is intentional though, because Thrift HMS and Glue will use different locking protocol, and so we will have different implementations of table operations. This will be made simpler if we flatten configuration, ie remove hive.metastore as config from Iceberg and configure everything via iceberg.catalog.type (#9577)

In this case, I wonder if we can just name this class as ThriftHiveTableOperations, where we can directly guarantee the existence of Thrift metastore, and for file metastore we don't need the stub, and we can have FileHiveTableOperations that inherit the same base class for all the shared functionalities.

That's a good point. I knew this refactor is imminent, but i didn't want to do this yet (understood you may be working on this already). Anyway, since you asked, i did that.

losipiuk

LGTM

findepi · 2021-10-11T13:29:34Z

CI #9300

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/HiveTableOperations.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java

losipiuk · 2021-10-12T09:46:38Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/HiveTableOperations.java

I did not notice originally that we are passing thriftMetastore for sake of obtaining "fresh" table here.
Maybe cleaner approach would be to extend HiveMetastore interface to have getTableUncached. Then for all but CachingHiveMetastore the method would just return getTable and for CachingHiveMetastore we would go straight to the backend.
Then you would not need to pass both HiveMetastore and ThriftMetastore to HiveTableOperations

Then you would not need to pass both HiveMetastore and ThriftMetastore to HiveTableOperations

For that you would also need to expose acquireTableExclusiveLock via HiveMetastore interface. But it is not bad I think.

Then you would not need to pass both HiveMetastore and ThriftMetastore to HiveTableOperations

To alleviate the need for this, i would also need to expose table lock/unlock functions in all HiveMetastore layers, which seems working against the abstraction we try to defend.

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/HiveTableOperations.java

testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergInsert.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastore.java

jackye1995 · 2021-10-13T18:25:33Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/HiveTableOperations.java

In this case, I wonder if we can just name this class as ThriftHiveTableOperations, where we can directly guarantee the existence of Thrift metastore, and for file metastore we don't need the stub, and we can have FileHiveTableOperations that inherit the same base class for all the shared functionalities.

This replaces `HiveTableOperations` with `HiveMetastoreTableOperations` and `FileMetastoreTableOperations` suitable for `HIVE_METASTORE` and `TESTING_FILE_METASTORE` Iceberg catalogs respectively, along with necessary interfaces.

jackye1995

overall looks good to me!

losipiuk · 2021-10-14T21:28:06Z

...-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractMetastoreTableOperations.java

nit: would be cool to have a separate commit which just inlines this into subclasses. And the do modifications in following commit.

Looks that FileMetastoreTableOperations is just inline and in Hive version there is locking added. But it is hard to see exaclty.

Extracted for visibility. Hive version looks like a bigger change, because nesting changed.

losipiuk

LGTM

jackye1995 · 2021-10-14T23:20:52Z

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/IcebergTableOperationsProvider.java

hdfsContext and queryId are all a part of the session, I think we can remove them from the interface.
HiveMetastore can be a part of the injected dependency, so we can probably also remove that.

sounds like potential follow up candidates, right?

actually, i misunderstood you, sorry. -> #9661

In the following commit, the implementation will diverge.

Previously, no locking was applied when writing Iceberg data, thus in case of concurrent writes (from same cluster, from multiple Trino clusters, or from different applications) successfully committed data could get made unreachable by a concurrent transaction's commit. This behavior is illustrated with a test being added here. Before the fix, the writes would always succeed, but the part of written data would not be visible in the final table state.

findepi · 2021-10-15T15:37:52Z

Added additional test in TestIcebergSparkCompatibility.

findepi · 2021-10-15T17:52:27Z

CI #9658

findepi requested review from jackye1995, losipiuk and martint October 11, 2021 10:36

cla-bot bot added the cla-signed label Oct 11, 2021

findepi commented Oct 11, 2021

View reviewed changes

losipiuk approved these changes Oct 11, 2021

View reviewed changes

findepi force-pushed the findepi/iceberg-hms-lock branch 2 times, most recently from 42b3544 to 2efe784 Compare October 11, 2021 11:23

findepi force-pushed the findepi/iceberg-hms-lock branch from 2efe784 to c3e5174 Compare October 11, 2021 13:32