Support force option on RegisterTable procedure #5327

yabola · 2022-07-21T11:46:15Z

fix #5163
Add 'force' option to register existing table, this is optional
usage: CALL mycatalogname.system.register_table('mycatalogname.mydb.mytablename','xx://xxxxx/metadata/xx.metadata.json','force')

yabola · 2022-07-21T15:34:39Z

@flyrain CC~

flyrain · 2022-07-26T00:16:33Z

Hi @RussellSpitzer @szehon-ho, could you approve the workflows?

flyrain · 2022-07-26T00:29:32Z

api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java

   * @return a Table instance
   * @throws AlreadyExistsException if the table already exists in the catalog.
   */
-  Table registerTable(SessionContext context, TableIdentifier ident, String metadataFileLocation);
+  Table registerTable(SessionContext context, TableIdentifier ident, String metadataFileLocation, boolean force);


It is a bit confusing for user by reusing the name registerTable for updating the metadata location, even with the force option. Also updating metadata location is a risky operation, we need to separate it from a safe operation(register a table), so that user won't accidentally do that. Suggested names would be resetTable or resetMetadataLocation.

@flyrain Do I need to introduce a new Spark procedure method, or just reuse the registerTable procedure?

I think a new procedure will be better. Would like to hear from others. @szehon-ho @RussellSpitzer.

yea I was initially thinking a separate one will be more clear .

I liked it being just an additional parameter, but I think I'm outvoted here :)

Based on @szehon-ho 's suggestion below perhaps this should be "resetTable" based on the forReset comments below

flyrain · 2022-07-26T00:33:53Z

api/src/main/java/org/apache/iceberg/catalog/Catalog.java

   * @return a Table instance
   * @throws AlreadyExistsException if the table already exists in the catalog.
   */
-  default Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+  default Table registerTable(TableIdentifier identifier, String metadataFileLocation, boolean force) {


The same concern here.

yabola · 2022-07-26T00:37:19Z

Recently merged new code and I need to resolve the conflict first

flyrain · 2022-07-26T00:41:41Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

      throw new org.apache.iceberg.exceptions.AlreadyExistsException("Table already exists: %s", identifier);
    }
+    HiveTableOperations ops = (HiveTableOperations) newTableOps(identifier);
+    TableMetadata base = ops.current();


Do we need to get the base? Passing the null should be fine, right?

If pass in base, we can change the commit method less, otherwise, the logic of commit checking meta will be incorrect. commit method means changing table meta from base to newMeta. I think this is good for maintaining the existing logic.

I feel that the changes here will have the least impact on the overall method, and no additional logic will be added. The connection between methods is also self-consistent

I'm not sure we should go through the operation.commit pathway here. Feels like what we are doing here is different than every other kind of commit. Really all we want to do is transactionally swap one parameter for another. I'm gonna think more about this tomorrow

flyrain · 2022-07-26T00:43:54Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

-    String newMetadataLocation = base == null && metadata.metadataFileLocation() != null ?
-        metadata.metadataFileLocation() : writeNewMetadata(metadata, currentVersion() + 1);
+    boolean isTableForRegister = metadata.isForRegister();
+    String newMetadataLocation = isTableForRegister ? metadata.metadataFileLocation()


Do we need to check if metadata.metadataFileLocation() is null?

Will not be null, only registerTable will take this value, the registerTable method has ensured that it is not null

szehon-ho · 2022-07-26T21:53:28Z

api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java

   * @return a Table instance
   * @throws AlreadyExistsException if the table already exists in the catalog.
   */
-  Table registerTable(SessionContext context, TableIdentifier ident, String metadataFileLocation);
+  Table registerTable(SessionContext context, TableIdentifier ident, String metadataFileLocation, boolean force);


yea I was initially thinking a separate one will be more clear .

szehon-ho · 2022-07-26T22:13:44Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

@@ -215,8 +215,9 @@ protected void doRefresh() {
  @SuppressWarnings("checkstyle:CyclomaticComplexity")
  @Override
  protected void doCommit(TableMetadata base, TableMetadata metadata) {
-    String newMetadataLocation = base == null && metadata.metadataFileLocation() != null ?
-        metadata.metadataFileLocation() : writeNewMetadata(metadata, currentVersion() + 1);
+    boolean isTableForRegister = metadata.isForRegister();


Should probably be called 'forReset()' or something more descriptive. This is a bit ugly but couldn't think of a good way to avoid it. I suppose just having a metadata with a location != null , regardless of whether base is null or not , is not enough to signal its a reset? @aokolnychyi @RussellSpitzer @rdblue for thoughts?

szehon-ho · 2022-07-26T22:14:51Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

@@ -215,8 +215,9 @@ protected void doRefresh() {
  @SuppressWarnings("checkstyle:CyclomaticComplexity")
  @Override
  protected void doCommit(TableMetadata base, TableMetadata metadata) {
-    String newMetadataLocation = base == null && metadata.metadataFileLocation() != null ?
-        metadata.metadataFileLocation() : writeNewMetadata(metadata, currentVersion() + 1);
+    boolean isTableForRegister = metadata.isForRegister();


Also seems like we should implement this for all the catalogs, or at least error there if this path is chosen.

ajantha-bhat · 2022-07-28T09:29:20Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

@@ -235,19 +235,21 @@ public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
  }

  @Override
-  public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+  public org.apache.iceberg.Table registerTable(TableIdentifier identifier,


please rebase.

#5037 has moved register table to BaseMetastoreCatalog

yabola · 2022-07-29T00:33:29Z

@flyrain @RussellSpitzer @szehon-ho @ajantha-bhat I had separated the resetTable API and do not reuse the commit method (from @RussellSpitzer Feels like what we are doing here is different than every other kind of commit.)
Do you think it is more clear? If it looks good to you, I will finish some other catalog resetTable API.

pvary · 2022-07-29T10:00:54Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+    try {
+      lockId = Optional.of(acquireLock());
+      Table tbl = loadHmsTable();


I do not like this. Too big part of the commit code is duplicated, and this is especially true for the error handling. This part is complicated and error prone, so we might want to try to find a better way to wave the to parts of the code together.

I want to abstract this part of the locking logic, let me think about it~

@pvary I abstract the part of the metadata locking logic. Please have a look.

rdblue · 2022-07-31T16:37:49Z

api/src/main/java/org/apache/iceberg/catalog/Catalog.java

+   * @param metadataFileLocation the location of a metadata file
+   * @return a Table instance
+   */
+  default Table resetTable(TableIdentifier identifier, String metadataFileLocation) {


Do we really need to add another method to Catalog? What is the difference between this and dropTable followed by registerTable?

But It may have a time gap, when there are consecutive queries. And it doesn't seem guaranteed to be a transactional operation

rdblue · 2022-07-31T16:39:24Z

I think we should be very careful about introducing new methods to the Catalog API. There needs to be a strong justification for adding to it, and I don't think merging this PR is a good idea.

yabola · 2022-08-01T03:32:50Z

I think we should be very careful about introducing new methods to the Catalog API. There needs to be a strong justification for adding to it, and I don't think merging this PR is a good idea.

How about reuse registerTable procedure?

rdblue · 2022-08-01T18:31:03Z

How about reuse registerTable procedure?

If you mean to add more arguments to registerTable so that it is an atomic operation, I don't really see much point to it. If this is a recovery operation for cases like the ones @szehon-ho listed in the issues, then why does it need to be atomic? The table is already broken.

We added registerTable as a way to migrate to a new metastore. At the time, there were concerns about people misusing it to fork tables and do other unreliable things. I still have that concern for this operation. Reusing registerTable as-is seems like the best way to avoid those problems.

flyrain · 2022-08-02T18:33:10Z

My concern for dropping table is that a catalog like HMS usually has hooks to react on events like a table is dropped. We don't really want to drop a table here, but it may trigger something unexpected.

RussellSpitzer · 2022-08-02T18:33:38Z

I prefer this approach (I think I said so before :) )

Yes fine you win :). Yea the only kind of downside is if we error re-creating the table, the table is dropped as the two are not atomic. But register can just be called again in that case. Agree its not worth the complexity to implement atomicity for a small gain.

I thought we could just switch the "create" to an "alter" and keep atomicity but I didn't look too much into that.

yabola · 2022-08-06T14:27:46Z

@szehon-ho as the discussion above, please take a look on my latest change. This modification can minimally invade the commit logic. And the original registerTable means table from null to a new table. New logic (drop then create) is also in line with the original logic.

rdblue · 2022-08-07T20:20:34Z

@szehon-ho, what I originally meant was that it would be fine for the caller to manually call dropTable first. I'm hesitant to add a force option because it's basically the same thing as a reset, we just hide it a bit.

Thinking about the problem a bit more, I'm not sure that the use cases we're trying to satisfy make this a catalog operation. If this is about replacing the metadata for a single existing table, why not make this a table operation? Or better yet, why not use the existing table operation to do this? If the table exists, then you can always call table.operations().commit(table.operations().current(), replacementMetadata).

RussellSpitzer · 2022-08-07T22:22:37Z

@rdblue The issue with those solutions for our use case is that they only work if the current metadata is valid. If not, we cannot drop the table via the Catalog api (requires loading the metadata first) which has led me to constantly showing folks how to access the hive via the Spark external client wrapper. The same issue occurs if we want to make this a table operation, we can't perform an operation on a table which doesn't have a valid metadata.json.

Our issue is that in Spark we can't Drop the table or point to a new metadata without using an api for the metastore directly. Ideally I think the Iceberg catalog implementation should provide an exit valve for these situations so users aren't required to manually connect to the underlying catalog system themselves.

I am open to other solutions here but I'm not very worried about folks overusing this api, it tends to only come up when the user is already in a really bad state.

yabola · 2022-08-08T10:00:53Z

But call table.operations().commit(table.operations().current(), replacementMetadata) can't update the metadata location (There is some check logic in this method that prevents changes).
I think this PR provides a command line way to accomplish this function.

rdblue · 2022-08-08T16:30:04Z

@RussellSpitzer, I see what you mean. What about fixing the Hive catalog so that the commit succeeds if table metadata can't be loaded? It sounds like we should have better handling for broken tables.

RussellSpitzer · 2022-08-08T16:35:23Z

@RussellSpitzer, I see what you mean. What about fixing the Hive catalog so that the commit succeeds if table metadata can't be loaded? It sounds like we should have better handling for broken tables.

That would be ok for me as well, I'm really just looking for a blessed way of fixing these states. I think allowing for dropping an non-iceberg or iceberg table with invalid metadata is probably ok too but that does seem a little scary to me than having an explicit register command which we know isn't part of normal operations.

szehon-ho · 2022-08-08T17:40:27Z

What about fixing the Hive catalog so that the commit succeeds if table metadata can't be loaded? It sounds like we should have better handling for broken tables.

Are we suggesting change dropTable to continue if table metadata cannot be loaded? (I assume we wont add any force flag?). That sounds like a great feature, its one of the pain points using Iceberg at least on Hive catalog, dropping a tables where metadata doesnt exist, would love to see a pr on that.

It's not the complete solution to the issue I initially filed (rollback corrupt metadata) though, and agree it still would be nice if there's a blessed way somehow

rdblue · 2022-08-08T17:54:17Z

@szehon-ho, in part, yes. I think that if we have situations where a table is corrupted then we should handle those in a better way than just having a table that requires manual intervention. Making DROP work for those tables sounds like a good idea to me. Maybe we could also update registerTable to overwrite a corrupt Iceberg table using similar logic. Then we wouldn't need a flag.

flyrain · 2022-08-08T18:38:15Z

+1 to detect if the table is corrupt first, then register the table with the new metadata file location. No flag is needed.

szehon-ho · 2022-08-08T21:54:29Z

Sounds good. The only corruption detection we can tell now is if the metadata.json no longer exists, I think, unless there are other things we can easily detect.

My initial thought for this pr was actually for non-corrupt case (ie, I have a metadata json that loads fine, but the metadata is wrong, eg issues listed in #5163). But I guess the consensus is , the user just writes a script to drop and register the Iceberg table with the right metadata json in that case, and we don't want to extend Iceberg Catalog at this point to handle those cases.

@yabola sorry about that, but I guess if you want to instead make a patch for the issue (make HiveCatalog.dropTable able to handle non-exist metadata json).

yabola · 2022-08-09T16:29:38Z

@szehon-ho Sure, I will make another PR for drop corrupt table. Thanks for all your discussions !

GabeChurch · 2023-03-31T22:07:43Z

I'm going to bump this as well.

It is potentially extremely helpful in cases where users want to "extend-read-only" s3-access to an iceberg table across environments, but have a separate hive metastores. For instance -- say users have a sandbox environment where they want to make a table from a production environment accessible that doesn't depend on atomicity, extending read-only access would save a HUGE expense in replication.

With the existing procedure the only option is to force drop the table before re-registering a new metafile, which can break other things attempting to access the table. It would be much better to maintain a lock on the table until the metadata has changed (drop+register at API level) so that other queries do not fail if they attempt to query while new metadata is being written via the force option mentioned.

If we can't add a "force" option to the "iceberg.system.register_table" we should add a new "non-storage-level-invasive" method to update metadata without requiring end users to drop a table via another system process -- maybe something like "iceberg.system.override_registered_table"

rushilshah1 · 2023-11-09T15:43:54Z

I am also interested in this functionality as well! Are there still plans to add this?

abfisher0417 · 2024-01-22T15:44:45Z

I am also interested in this capability. I have a use case where I create/update Iceberg tables in an isolated cloud environment separate from consumers/readers of those tables. Consumers would periodically sync all metadata/data files from the producing cloud storage bucket to their own cloud storage bucket; consumers would also have their own catalog(s) that point to the underlying table metadata/data files.

The RegisterTable procedure today would allow partners in this scenario to add tables to their own catalog. However, there's not a "clean" way to point an existing table to a newer version of the same table without first dropping the table the re-adding it.

yabola · 2024-01-23T02:36:01Z

@abfisher0417 Thank you. If community agrees, I can complete this PR again.

github-actions · 2024-08-17T00:13:11Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions bot added API core hive spark labels Jul 21, 2022

flyrain reviewed Jul 26, 2022

View reviewed changes

szehon-ho reviewed Jul 26, 2022

View reviewed changes

ajantha-bhat reviewed Jul 28, 2022

View reviewed changes

yabola force-pushed the register branch from 375d69c to 2e03e7f Compare July 29, 2022 00:25

github-actions bot added AWS build DELL NESSIE labels Jul 29, 2022

yabola force-pushed the register branch from 2e03e7f to 7946c9e Compare July 29, 2022 00:28

github-actions bot removed AWS NESSIE DELL labels Jul 29, 2022

pvary reviewed Jul 29, 2022

View reviewed changes

yabola changed the title ~~Support force option on registerTable procedure~~ Support resetTable procedure Jul 30, 2022

yabola force-pushed the register branch from 7946c9e to 9f47165 Compare July 31, 2022 15:07

rdblue reviewed Jul 31, 2022

View reviewed changes

Support force option on RegisterTable procedure

4b22c05

yabola force-pushed the register branch from 9f47165 to 4b22c05 Compare August 6, 2022 14:23

github-actions bot added AWS DELL NESSIE labels Aug 6, 2022

Support force option on RegisterTable procedure

8d598f3

yabola changed the title ~~Support resetTable procedure~~ Support force option on RegisterTable procedure Aug 6, 2022

Support delete corrupted Iceberg table

2063631

ajantha-bhat mentioned this pull request Feb 4, 2023

AWS: support force register table in GlueCatalog #6742

Closed

github-actions bot added the stale label Aug 17, 2024

yabola closed this Aug 19, 2024

dramaticlly mentioned this pull request Feb 10, 2025

RESTTableOperations does not support table metadata swap like others TableOperations did #12134

Open

3 tasks

Support force option on RegisterTable procedure #5327

Support force option on RegisterTable procedure #5327

Conversation

yabola commented Jul 21, 2022 • edited Loading

yabola commented Jul 21, 2022

flyrain commented Jul 26, 2022

Choose a reason for hiding this comment

yabola Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yabola commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yabola commented Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Jul 31, 2022

yabola commented Aug 1, 2022

rdblue commented Aug 1, 2022

flyrain commented Aug 2, 2022 • edited Loading

RussellSpitzer commented Aug 2, 2022

yabola commented Aug 6, 2022 • edited Loading

rdblue commented Aug 7, 2022

RussellSpitzer commented Aug 7, 2022

yabola commented Aug 8, 2022

rdblue commented Aug 8, 2022

RussellSpitzer commented Aug 8, 2022

szehon-ho commented Aug 8, 2022

rdblue commented Aug 8, 2022

flyrain commented Aug 8, 2022

szehon-ho commented Aug 8, 2022

yabola commented Aug 9, 2022

GabeChurch commented Mar 31, 2023 • edited Loading

rushilshah1 commented Nov 9, 2023

abfisher0417 commented Jan 22, 2024

yabola commented Jan 23, 2024

github-actions bot commented Aug 17, 2024

yabola commented Jul 21, 2022 •

edited

Loading

yabola Jul 26, 2022 •

edited

Loading

yabola commented Jul 29, 2022 •

edited

Loading

flyrain commented Aug 2, 2022 •

edited

Loading

yabola commented Aug 6, 2022 •

edited

Loading

GabeChurch commented Mar 31, 2023 •

edited

Loading