You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/09/16 11:35:26 UTC
[GitHub] [iceberg] pvary opened a new pull request #1465: Core: Enhance version-hint.txt recovery with file listing
pvary opened a new pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465
The patch enhances the recovery introduced in #1443 for missing/corrupted version-hint.txt by listing the files in the metadata directory and returning the highest possible version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jacques-n commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489613609
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
I'd go one step further. I think a user should have to opt into this fallback behavior. I think the default should be to throw an exception. Silently "corrupting" the table isn't something that should happen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489605866
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java
##########
@@ -494,23 +495,42 @@ public void testVersionHintFile() throws Exception {
stream.write("3".getBytes(StandardCharsets.UTF_8));
}
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(3, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
// Write an empty version hint file
io.deleteFile(versionHintLocation);
io.newOutputFile(versionHintLocation).create().close();
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(1, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
// Just delete the file - double check that we have manipulated the correct file
io.deleteFile(versionHintLocation);
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(1, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
+
+ // Remove the first version file, and see if we can recover
+ io.deleteFile(tableOperations.getMetadataFile(1).toString());
+
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
+ Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
+
+ // Remove all the version files, and see if we can recover. Hint... not :)
+ io.deleteFile(tableOperations.getMetadataFile(2).toString());
+ io.deleteFile(tableOperations.getMetadataFile(3).toString());
+
+ // Check that we got 0 versionHint, and a NoSuchTableException is thrown when trying to load the table
+ Assert.assertEquals(0, tableOperations.versionHint());
+ AssertHelpers.assertThrows(
+ "Should not be able to find the table",
+ NoSuchTableException.class,
+ "Table does not exist: tbl",
+ () -> catalog.loadTable(tableId));
Review comment:
Since this is a new feature, can you put it in its own test method?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490128518
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ version = version(file.getPath().getName());
+ if (version > maxVersion && getMetadataFile(version) != null) {
+ maxVersion = version;
+ }
}
+ return maxVersion;
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#issuecomment-693702787
This fails locally for me in `TestHadoopCatalog` line 508 (not 522). The problem is that `version` is set as a side-effect of `readHint`. As a result, `version` is equal to the new version passed to `updateVersionAndMetadata` so that method assumes that there has been no change and returns the current metadata (`null`) without reading the metadata file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r491128772
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
> Could someone please point me a place (code lines) where and how Iceberg configurations are handled, so I can use them correctly?
I don't think we need this option since Jacques replied that he agrees it should not be an exception.
For configuration, we prefer to use table properties for most configuration, which are defined in the `TableProperties` class. For other cases, we use the configuration that makes the most sense. Here, I would use the Hadoop configuration because the table implementation is for Hadoop and HDFS.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490840606
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -280,7 +302,7 @@ private void writeVersionHint(int versionToWrite) {
}
@VisibleForTesting
- int readVersionHint() {
+ int versionHint() {
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490437865
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -280,7 +302,7 @@ private void writeVersionHint(int versionToWrite) {
}
@VisibleForTesting
- int readVersionHint() {
+ int versionHint() {
Review comment:
I think a better name for this is `findVersion` because it will do listing now. It isn't really a hint because it handles the case where the hint is missing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] fbocse commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489704855
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
@rdblue we've seen this happening with hadoop implementations that implement `create(overwrite=true)` by a server-side `delete` and `create` operation - so for a split micro-second we get a 404 on the version-hint.text file - we've looked at solving this in a pretty similar file but instead of a fallback mechanism we went with the approach that a table configuration (or version?) indicates the version resolver implementation, either file based or directory listing based.
@jacques-n I don't think that this approach to fallback to directory listing in case of failing to resolve the version hint file can lead to silent "corruption" of the table. Can you think of such a scenario? I'm asking cause we were considering doing something similar so I'm interested in any thoughts that may question this approach.
But to your point if there so much is a possibility that this approach can lead to table corruption then we should probably not provide this feature at all, let alone say support it by a "turn-this-feature-at-own-risk" approach - it will still corrupt the table :)
@pvary Big difference in the listing directory vs file approach is the constant time in loading data from a file (constant time) vs using hadoop listing directory (probably not constant time).
I would assume it would take longer to resolve the version and probably it would be advised that older versions are dropped so the list API provides a decent latency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490161818
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ version = version(file.getPath().getName());
+ if (version > maxVersion && getMetadataFile(version) != null) {
+ maxVersion = version;
+ }
}
+ return maxVersion;
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490162301
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java
##########
@@ -494,23 +495,42 @@ public void testVersionHintFile() throws Exception {
stream.write("3".getBytes(StandardCharsets.UTF_8));
}
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(3, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
// Write an empty version hint file
io.deleteFile(versionHintLocation);
io.newOutputFile(versionHintLocation).create().close();
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(1, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
// Just delete the file - double check that we have manipulated the correct file
io.deleteFile(versionHintLocation);
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(1, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
+
+ // Remove the first version file, and see if we can recover
+ io.deleteFile(tableOperations.getMetadataFile(1).toString());
+
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
+ Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
+
+ // Remove all the version files, and see if we can recover. Hint... not :)
+ io.deleteFile(tableOperations.getMetadataFile(2).toString());
+ io.deleteFile(tableOperations.getMetadataFile(3).toString());
+
+ // Check that we got 0 versionHint, and a NoSuchTableException is thrown when trying to load the table
+ Assert.assertEquals(0, tableOperations.versionHint());
+ AssertHelpers.assertThrows(
+ "Should not be able to find the table",
+ NoSuchTableException.class,
+ "Table does not exist: tbl",
+ () -> catalog.loadTable(tableId));
Review comment:
Done with the refactoring.
Feel free to request changes in the new version if it could be further improved
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#issuecomment-695028373
Looks good to me! I merged this.
I agree that we can follow up with an improvement to avoid corrupt version hints. No need to mix that in here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490852704
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
> I think @jacques-n has a good point. The version file should not be corrupt and we should make sure of it by changing how we write the file. Since this is for HDFS, creating the file with an atomic rename to ensure the entire file is written before making it the current version hint makes sense to me. That way we don't get dirty reads.
I think that is something for another PR.
> It would still be good to have an info log when the file is missing and there is no metadata (table doesn't exist), and a warning if the file is missing but a metadata file is found.
Here is my proposed solution:
- No metadata directory: DEBUG log - there are some perfectly valid cases where we expect that the directory/table is not there. Higher level log message could be very confusing (Seen very bad examples in Hive code 😢)
- Existing metadata directory, no hint file: WARN log - transient error, or hint file error. Good to be aware of.
- IOException while checking for existence of the metadata directory or listing: WARN log - Some fs level exception. Has to be checked.
Your thoughts?
> Last, I'm for an option to throw an exception when the hint file is corrupt rather than throwing a warning, but the hint file is not intended as a requirement for correctness, so I'd turn this off by default.
Could someone please point me a place (code lines) where and how Iceberg configurations are handled, so I can use them correctly?
Thanks,
Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] fbocse commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489704855
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
@rdblue we've seen this happening with hadoop implementations that implement `create(overwrite=true)` by a server-side `delete` and `create` operation - so the API isn't consistent - for a split micro-second we get a 404 on the version-hint.text file - we're covering for those with retries - however they do provide read-after-write guarantees on directory listing - so we've looked at solving this in a pretty similar fashion, but instead of a fallback mechanism we went with the approach that a table configuration (or version?) indicates the version resolver implementation, either file based or directory listing based, but I think this approach is actually better...
@jacques-n I don't think that this approach to fallback to directory listing in case of failing to resolve the version hint file can lead to silent "corruption" of the table. Can you think of such a scenario? I'm asking cause we were considering doing something similar so I'm interested in any thoughts that may question this approach.
But to your point if there so much is a possibility that this approach can lead to table corruption then we should probably not provide this feature at all, let alone say support it by a "turn-this-feature-at-own-risk" approach - it will still corrupt the table :)
@pvary Big difference in the listing directory vs file approach is the constant time in loading data from a file (constant time) vs using hadoop listing directory (probably not constant time).
I would assume it would take longer to resolve the version and probably it would be advised that older versions are dropped so the list API provides a decent latency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] fbocse commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489711939
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
Review comment:
This will list all the files in the metadata directory, right? It's just that we're filtering on the client for the files matching the version naming pattern.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489605263
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ version = version(file.getPath().getName());
+ if (version > maxVersion && getMetadataFile(version) != null) {
+ maxVersion = version;
+ }
}
+ return maxVersion;
Review comment:
Nit: we like to have newlines after control flow statements/blocks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jacques-n commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490632123
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
Looking at the code, I was wrong about the table corruption. I thought that the version hint file was used in a slightly different manner. I agree with @rdblue that the default probably shouldn't be exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489621570
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
The warn message appeared several times for me and I have realized that we call this method even for new tables to check that the data/metadata is not yet there, so this warning message could be misleading.
@jacques-n: Adding a configuration seems like a good idea because this recovery mechanisms starts to become complicated. Could you help by pointing out some similar configurable solutions from where I can start from?
Thanks,
Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490131562
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ version = version(file.getPath().getName());
Review comment:
I was so focused on not finding a way to replicate the failures on my side, that I missed this.
Big KUDOS for catching this! :)
I hope fixing this will fix the tests as well (Still not sure why they were passing on my side 😢)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490437323
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
I think @jacques-n has a good point. The version file should not be corrupt and we should make sure of it by changing how we write the file. Since this is for HDFS, creating the file with an atomic rename to ensure the entire file is written before making it the current version hint makes sense to me. That way we don't get dirty reads.
Doing that would result in cases like @fbocse brings up: we'd have to delete the version hint file to rename into its place. And there are cases where the hint file is missing, like when trying to load a table to check whether it exists. In that case, I don't think that it makes sense to warn when a hint file is missing, only when it is corrupt. It would still be good to have an info log when the file is missing and there is no metadata (table doesn't exist), and a warning if the file is missing but a metadata file is found.
Last, I'm for an option to throw an exception when the hint file is corrupt rather than throwing a warning, but the hint file is not intended as a requirement for correctness, so I'd turn this off by default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] fbocse commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489704855
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
@rdblue we've seen this happening with hadoop implementations that implement `create(overwrite=true)` by a server-side `delete` and `create` operation - so for a split micro-second we get a 404 on the version-hint.text file - we've looked at solving this in a pretty similar file but instead of a fallback mechanism we went with the approach that a table configuration (or version?) indicates the version resolver implementation, either file based or directory listing based.
@jacques-n I don't think that this approach to fallback to directory listing in case of failing to resolve the version hint file can lead to silent "corruption" of the table. Can you think of such a scenario? I'm asking cause we were considering doing something similar so I'm interested in any thoughts that may question this approach.
But to your point if there so much is a possibility that this approach can lead to table corruption then we should probably not provide this feature at all, let alone say support it by a "turn-this-feature-at-own-risk" approach - it will still corrupt the table :)
@pvary Big difference in the listing directory vs file approach is the constant time in loading data from a file (constant time) vs using hadoop listing directory (probably not constant time).
I would assume it would take longer to resolve the version and probably it would be advised that older versions are dropped so the list API provide a decent latency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] fbocse commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489704855
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
@rdblue we've seen this happening with hadoop implementations that implement `create(overwrite=true)` by a server-side `delete` and `create` operation - so for a split micro-second we get a 404 on the version-hint.text file - so the API isn't consistent - however they do provide read-after-write guarantees on directory listing - so we've looked at solving this in a pretty similar fashion, but instead of a fallback mechanism we went with the approach that a table configuration (or version?) indicates the version resolver implementation, either file based or directory listing based, but I think this approach is actually better...
@jacques-n I don't think that this approach to fallback to directory listing in case of failing to resolve the version hint file can lead to silent "corruption" of the table. Can you think of such a scenario? I'm asking cause we were considering doing something similar so I'm interested in any thoughts that may question this approach.
But to your point if there so much is a possibility that this approach can lead to table corruption then we should probably not provide this feature at all, let alone say support it by a "turn-this-feature-at-own-risk" approach - it will still corrupt the table :)
@pvary Big difference in the listing directory vs file approach is the constant time in loading data from a file (constant time) vs using hadoop listing directory (probably not constant time).
I would assume it would take longer to resolve the version and probably it would be advised that older versions are dropped so the list API provides a decent latency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490130511
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
Review comment:
Sadly, yes. I did not find a better solution where the filter is pushed to server side.
Any ideas how to do it more efficiently?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] fbocse commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489704855
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
@rdblue we've seen this happening with hadoop implementations that implement `create(overwrite=true)` by a server-side `delete` and `create` operation - so the API isn't consistent - for a split micro-second we get a 404 on the version-hint.text file - we're covering for those with retries - however they do provide read-after-write guarantees on directory listing - so we've looked at solving this in a pretty similar fashion, but instead of a fallback mechanism we went with the approach that a table configuration (or version?) indicates the version resolver implementation, either file based or directory listing based (each version is created as a new, version named file with `create(overwrite=false)`, just like the version files, in the versions directory and the same listing is applied to pick the highest ranking), but I think this approach is actually better cause it's simple however it doesn't control the latency of the list API...
@jacques-n I don't think that this approach to fallback to directory listing in case of failing to resolve the version hint file can lead to silent "corruption" of the table. Can you think of such a scenario? I'm asking cause we were considering doing something similar so I'm interested in any thoughts that may question this approach.
But to your point if there so much is a possibility that this approach can lead to table corruption then we should probably not provide this feature at all, let alone say support it by a "turn-this-feature-at-own-risk" approach - it will still corrupt the table :)
@pvary Big difference in the listing directory vs file approach is the constant time in loading data from a file (constant time) vs using hadoop listing directory (probably not constant time).
I would assume it would take longer to resolve the version and probably it would be advised that older versions are dropped so the list API provides a decent latency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489788459
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ version = version(file.getPath().getName());
Review comment:
I didn't catch this at first, but this method should not have a side effect of setting `version`. It should return version and something else should set it.
Also, when we set instance fields, we use the prefix `this.` so that it is clear that an instance field and not a local variable is set. That's why I missed this in my first round of review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490129106
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java
##########
@@ -494,23 +495,42 @@ public void testVersionHintFile() throws Exception {
stream.write("3".getBytes(StandardCharsets.UTF_8));
}
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(3, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
// Write an empty version hint file
io.deleteFile(versionHintLocation);
io.newOutputFile(versionHintLocation).create().close();
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(1, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
// Just delete the file - double check that we have manipulated the correct file
io.deleteFile(versionHintLocation);
- // Check the result of the readVersionHint(), and load the table and check the current snapshotId
- Assert.assertEquals(1, tableOperations.readVersionHint());
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
+
+ // Remove the first version file, and see if we can recover
+ io.deleteFile(tableOperations.getMetadataFile(1).toString());
+
+ // Check the result of the versionHint(), and load the table and check the current snapshotId
+ Assert.assertEquals(3, tableOperations.versionHint());
+ Assert.assertEquals(secondSnapshotId, catalog.loadTable(tableId).currentSnapshot().snapshotId());
+
+ // Remove all the version files, and see if we can recover. Hint... not :)
+ io.deleteFile(tableOperations.getMetadataFile(2).toString());
+ io.deleteFile(tableOperations.getMetadataFile(3).toString());
+
+ // Check that we got 0 versionHint, and a NoSuchTableException is thrown when trying to load the table
+ Assert.assertEquals(0, tableOperations.versionHint());
+ AssertHelpers.assertThrows(
+ "Should not be able to find the table",
+ NoSuchTableException.class,
+ "Table does not exist: tbl",
+ () -> catalog.loadTable(tableId));
Review comment:
Done.
Create a new method for this.
Might refactor a little bit later to have a nicer code, but first I would like to be sure that the failing tests are solved
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490128410
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
> I think this approach is actually better cause it's simple however it doesn't control the latency of the list API...
If I understand correctly in HadoopTableOperations.refresh() we use the number provided by versionHint() only as a basis for checking newer versions. Would that cover your concerns?
```
Path nextMetadataFile = getMetadataFile(ver + 1);
while (nextMetadataFile != null) {
ver += 1;
metadataFile = nextMetadataFile;
nextMetadataFile = getMetadataFile(ver + 1);
}
updateVersionAndMetadata(ver, metadataFile.toString());
```
> @pvary Big difference in the listing directory vs file approach is the constant time in loading data from a file (constant time) vs using hadoop listing directory (probably not constant time).
With the current patch we still do the fast / file based version resolving, and only fall back to the listing based version if there is a problem with the version-hint.txt file.
> I would assume it would take longer to resolve the version and probably it would be advised that older versions are dropped so the list API provides a decent latency.
Based on my current understanding the older versions might be needed for timetravel or other features. It should be really the decision of the users to decide when they want to remove them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490438056
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,28 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ if (fs.exists(metadataRoot())) {
+ LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ }
+
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ int currentVersion = version(file.getPath().getName());
+ if (currentVersion > maxVersion && getMetadataFile(currentVersion) != null) {
+ maxVersion = currentVersion;
+ }
}
+
+ return maxVersion;
} catch (IOException io) {
// We log this error only on debug level since this is just a problem in recovery path
LOG.debug("Error trying to recover version-hint.txt data for {}", versionHintFile, e);
+ return 0;
Review comment:
I think it is more clear that the return value is only used when there is an exception in the recovery path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490429646
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,28 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ if (fs.exists(metadataRoot())) {
+ LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ }
+
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ int currentVersion = version(file.getPath().getName());
+ if (currentVersion > maxVersion && getMetadataFile(currentVersion) != null) {
+ maxVersion = currentVersion;
+ }
}
+
+ return maxVersion;
} catch (IOException io) {
// We log this error only on debug level since this is just a problem in recovery path
LOG.debug("Error trying to recover version-hint.txt data for {}", versionHintFile, e);
+ return 0;
Review comment:
Nit: This change isn't necessary?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r490162532
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
try {
- if (getMetadataFile(1) != null) {
- // We just assume corrupted metadata and start to read from the first version file
- return 1;
+ // List the metadata directory to find the version files, and try to recover the max available version
+ FileStatus[] files = fs.listStatus(metadataRoot(), name -> VERSION_PATTERN.matcher(name.getName()).matches());
+ int maxVersion = 0;
+
+ for (FileStatus file : files) {
+ version = version(file.getPath().getName());
Review comment:
This fixed the tests. Still not sure why my tests did not repro the case 😢
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489604332
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
Why make this debug instead of warn? I think it is still concerning that the version hint couldn't be found.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] fbocse commented on a change in pull request #1465: Core: Enhance version-hint.txt recovery with file listing
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #1465:
URL: https://github.com/apache/iceberg/pull/1465#discussion_r489704855
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java
##########
@@ -289,19 +311,24 @@ int readVersionHint() {
return Integer.parseInt(in.readLine().replace("\n", ""));
} catch (Exception e) {
- LOG.warn("Error reading version hint file {}", versionHintFile, e);
+ LOG.debug("Error reading version hint file {}", versionHintFile, e);
Review comment:
@rdblue we've seen this happening with hadoop implementations that implement `create(overwrite=true)` by a server-side `delete` and `create` operation - so for a split micro-second we get a 404 on the version-hint.text file - so the API isn't consistent - however they do provide read-after-write guarantees on directory listing - so we've looked at solving this in a pretty similar fashion, but instead of a fallback mechanism we went with the approach that a table configuration (or version?) indicates the version resolver implementation, either file based or directory listing based.
@jacques-n I don't think that this approach to fallback to directory listing in case of failing to resolve the version hint file can lead to silent "corruption" of the table. Can you think of such a scenario? I'm asking cause we were considering doing something similar so I'm interested in any thoughts that may question this approach.
But to your point if there so much is a possibility that this approach can lead to table corruption then we should probably not provide this feature at all, let alone say support it by a "turn-this-feature-at-own-risk" approach - it will still corrupt the table :)
@pvary Big difference in the listing directory vs file approach is the constant time in loading data from a file (constant time) vs using hadoop listing directory (probably not constant time).
I would assume it would take longer to resolve the version and probably it would be advised that older versions are dropped so the list API provides a decent latency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org