API: Support registering filesystem tables to metastore based catalogs #4946

ajantha-bhat · 2022-06-02T12:39:28Z

Filesystem tables and metastore tables have different naming conventions for TableMetadata file as per spec.
So, when a filesystem table is registered to hive catalog (or any metastore based catalog), the register was successful but loading the table used to fail in parseVersion as it was expecting the naming convention of metastore table.

Fixed it by using metastore table naming convention while registering.

Fixes #4794

rdblue · 2022-06-05T17:55:21Z

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

  }

+  private boolean isMetastoreTable(String metadataFileLocation) {
+    return metadataFileLocation.substring(metadataFileLocation.lastIndexOf("/") + 1).contains("-");


I think this should be more specific. Why not confirm that there's a number using a regex?

rdblue · 2022-06-05T17:57:59Z

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

    TableMetadata metadata = TableMetadataParser.read(ops.io(), metadataFile);
+
+    if (!isMetastoreTable(metadataFileLocation)) {
+      // Metastore table and filesystem tables will have different naming conventions as per the spec.


This seems unnecessary. Why not just update parseVersion to return -1 if the version isn't valid? I think that's a better solution than rewriting the metadata.

I like this suggestion. For filesystem tables, along with fixing register problem for hive, it can fix register problem for other catalogs also once they support it.

pvary · 2022-06-05T18:04:16Z

I think this again is a step in the direction of registering / adding the same table / snapshot to different catalogs. This will cause issues when ExpireSnapshots or the RemoveOrphanFiles processes are running. When some files are not needed in one catalog anymore they might be removed, and they will be missing from the other version later.

Do we have a solution for prevent this situation? Or do I miss something?

rdblue · 2022-06-05T18:09:18Z

@pvary, I think that this is a case that makes sense for the register method. We highly recommend using a real metastore rather than using the Hadoop strategy. To convert from a Hadoop catalog to a metastore catalog, you'd need this to work. To me, the register table method is mainly for migration between catalogs.

pvary · 2022-06-05T18:14:43Z

@rdblue: Putting that in this way, and I agree with you. If this is a migration, then this is the way to go.

Thanks for the explanation!

ajantha-bhat · 2022-06-06T04:18:19Z

@pvary : Yes, this was for migration purposes. As we are aware that keeping the same table in two catalog is dangerous and leads to data mismatch. The user who migrates needs to take care of deleting the table in source catalog using drop table with purge=false option using the catalog interface.

@rdblue : Thanks for clarification and review. I will address the comments.

ajantha-bhat · 2022-06-06T05:44:15Z

@rdblue : I have addressed the comments. Thanks.

rdblue · 2022-06-06T17:33:47Z

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

    try {
      return Integer.valueOf(metadataLocation.substring(versionStart, versionEnd));
-    } catch (NumberFormatException e) {
+    } catch (NumberFormatException | StringIndexOutOfBoundsException e) {


Why not check versionEnd? If versionEnd is < 0, then return -1. No need to catch an exception.

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java

rdblue · 2022-06-06T17:37:16Z

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java

+            () -> catalog.loadTable(identifier));
+
+    // register the table to hive catalog using the latest metadata file
+    metadataFiles.sort(Collections.reverseOrder());


Rather than guessing which file is the metadata file, I think you should just get the location from the table:

String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java

ajantha-bhat · 2022-06-07T02:44:28Z

@rdblue : Thanks for the review again. I have addressed all the comments.

ajantha-bhat · 2022-06-13T02:17:57Z

ping @rdblue

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

pvary · 2022-06-13T09:45:59Z

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

    int versionStart = metadataLocation.lastIndexOf('/') + 1; // if '/' isn't found, this will be 0
    int versionEnd = metadataLocation.indexOf('-', versionStart);
+    if (versionEnd < 0) {
+      // As per the spec, metastore tables and filesystem tables


What about something like this:

If there the version is not parseable return -1 as a sign that the metadata is not part of this catalog

Or ever better remove this comment and add a javadoc for this method, since this is the same behaviour as we handle NumberFormatExceptions

ajantha-bhat · 2022-06-13T11:21:25Z

@pvary : I have addressed the comments. Please check. Thanks.

pvary

+1 from my side. Let's see if @rdblue has other comments, or not. If not I will merge this around at the end of the week.

Thanks @ajantha-bhat!

rdblue · 2022-06-14T03:24:42Z

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

    int versionStart = metadataLocation.lastIndexOf('/') + 1; // if '/' isn't found, this will be 0
    int versionEnd = metadataLocation.indexOf('-', versionStart);
+    if (versionEnd < 0) {
+      LOG.warn("Found filesystem table's metadata : {}", metadataLocation);


I don't think this is a good thing to warn about. Warnings are for potential problems, while this is a normal case if you convert a table or control a name some other way.

This is also inaccurate. We can't really know why the file didn't include a version and shouldn't guess.

removed the log warning and added it as a code comment.

The more we talk about this, the less convinced I am that we should differentiate between the parsing errors. Nothing really prevents a developer to create their own TableOperations and using their own metadata filename format.

I think it would be good to have a log for the event in any case, at least in info level.

Maybe rewrite the whole parsing part to this:

private static int parseVersion(String metadataLocation) { try { int versionStart = metadataLocation.lastIndexOf('/') + 1; // if '/' isn't found, this will be 0 int versionEnd = metadataLocation.indexOf('-', versionStart); return Integer.valueOf(metadataLocation.substring(versionStart, versionEnd)); } catch (Throwable e) { LOG.info("Unable to parse version from metadata location: {}", metadataLocation, e); return -1; } }

What do you think?

SGTM. waiting for @rdblue 's opinion to avoid me working back and forth on this.

@pvary : As there was no response from @rdblue, I went ahead with your suggestion and modified the code.

I think this should check for versionEnd. That indicates that we're using a file with no version and should return -1.

If we also want to return -1 for NumberFormatException and warn, I'm all for that. But we should definitely not catch Throwable. That's dangerous. Let's handle the cases we know about.

Thanks for the inputs @rdblue. Agree that catching Throwable is not a good practice. I have reverted the latest change. So, final code is same as what you described above.

pvary · 2022-06-21T14:40:34Z

@pvary : I have addressed the comments. Please check. Thanks.

Let's give him a few more days, then I will merge.
Thanks, Peter

rdblue · 2022-06-29T18:39:51Z

Thanks, @ajantha-bhat!

github-actions bot added core hive labels Jun 2, 2022

rdblue reviewed Jun 5, 2022

View reviewed changes

pvary mentioned this pull request Jun 6, 2022

Hive: Fix an error when create external table in hive catalog #4916

Closed

rdblue reviewed Jun 6, 2022

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java Show resolved Hide resolved

rdblue reviewed Jun 6, 2022

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java Show resolved Hide resolved

ajantha-bhat changed the title ~~API: Support registering filesystem tables to hive catalog~~ API: Support registering filesystem tables to metastore based catalogs Jun 7, 2022

pvary reviewed Jun 13, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java Show resolved Hide resolved

pvary reviewed Jun 13, 2022

View reviewed changes

ajantha-bhat force-pushed the catalog branch from ca7cf58 to 96f5f8e Compare June 13, 2022 11:20

pvary approved these changes Jun 13, 2022

View reviewed changes

rdblue reviewed Jun 14, 2022

View reviewed changes

ajantha-bhat force-pushed the catalog branch from 96f5f8e to 108aeeb Compare June 14, 2022 03:40

ajantha-bhat added 3 commits June 21, 2022 19:24

API: Support registering filesystem tables to hive catalog

666d1fb

Review comment fixes

136d0b7

review comments round 2

ad577ec

ajantha-bhat force-pushed the catalog branch from 108aeeb to 86c8417 Compare June 21, 2022 13:56

rdblue added this to the Iceberg 0.14.0 Release milestone Jun 29, 2022

ajantha-bhat force-pushed the catalog branch from 86c8417 to ad577ec Compare June 29, 2022 17:11

rdblue approved these changes Jun 29, 2022

View reviewed changes

rdblue merged commit 3663497 into apache:master Jun 29, 2022

namrathamyske pushed a commit to namrathamyske/iceberg that referenced this pull request Jul 10, 2022

API: Fix version exception when registering metadata (apache#4946)

3f5b65a

namrathamyske pushed a commit to namrathamyske/iceberg that referenced this pull request Jul 10, 2022

API: Fix version exception when registering metadata (apache#4946)

1270788

ajantha-bhat mentioned this pull request Aug 10, 2022

Core: Support bulk table migration from one catalog to another #5492

Closed

InvisibleProgrammer mentioned this pull request Dec 16, 2022

HIVE-26822: port changes before spotless apache/hive#3857

Closed

API: Support registering filesystem tables to metastore based catalogs #4946

API: Support registering filesystem tables to metastore based catalogs #4946

Uh oh!

Conversation

ajantha-bhat commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvary commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue commented Jun 5, 2022

Uh oh!

pvary commented Jun 5, 2022

Uh oh!

ajantha-bhat commented Jun 6, 2022

Uh oh!

ajantha-bhat commented Jun 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajantha-bhat commented Jun 7, 2022

Uh oh!

ajantha-bhat commented Jun 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat commented Jun 13, 2022

Uh oh!

pvary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvary commented Jun 21, 2022

Uh oh!

rdblue commented Jun 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajantha-bhat commented Jun 2, 2022 •

edited

Loading

pvary commented Jun 5, 2022 •

edited

Loading