amogh-jahagirdar commented on code in PR #9498:
URL: https://github.com/apache/iceberg/pull/9498#discussion_r1456088754
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -360,7 +360,10 @@ int findVersion() {
*/
private void renameToFinal(FileSystem fs, Path src, Path dst, int
nextVersion) {
try {
- lockManager.acquire(dst.toString(), src.toString());
+ boolean success = lockManager.acquire(dst.toString(), src.toString());
+ if (!success) {
+ throw new CommitFailedException("Failed to acquire lock on file: %s",
dst);
+ }
Review Comment:
Style nit, if we could add a new line after this `if` block separating it
from the next `if`! I know in this file it looks like we didn't really follow
our normal practice already but at least for new changes we could follow it.
Also I think I'd just inline the boolean so
```
if (!lockManager.acquire(dst.toString(), src.toStrnig()) {
throw ...
}
```
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -360,7 +360,10 @@ int findVersion() {
*/
private void renameToFinal(FileSystem fs, Path src, Path dst, int
nextVersion) {
try {
- lockManager.acquire(dst.toString(), src.toString());
+ boolean success = lockManager.acquire(dst.toString(), src.toString());
+ if (!success) {
+ throw new CommitFailedException("Failed to acquire lock on file: %s",
dst);
+ }
Review Comment:
Also should we include the owner (src) in the exception as well?
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -451,4 +455,39 @@ public void testConcurrentFastAppends(@TempDir File dir)
throws Exception {
Assertions.assertThat(Lists.newArrayList(tableWithHighRetries.snapshots()))
.hasSize(threadsCount * numberOfCommitedFilesPerThread);
}
-}
+
+ @Test
+ public void testCommitFailedToAcquire() {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ Configuration conf = new Configuration();
+ LockManager lockManager = new NoLockManager();
+ HadoopTableOperations tops =
Review Comment:
Naming nit: I think it's better to be a bit more verbose rather than
truncate, since it's more readable imo. Instead of `tops` could we call it
`tableOperations` like we do in other places? Also maybe
`testCommitFailedToAcquireLock` for the test name?
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -383,7 +386,10 @@ private void renameToFinal(FileSystem fs, Path src, Path
dst, int nextVersion) {
}
throw cfe;
} finally {
- lockManager.release(dst.toString(), src.toString());
+ boolean success = lockManager.release(dst.toString(), src.toString());
+ if (!success) {
+ LOG.warn("Failed to release lock on file: {} with owner: {}", dst,
src);
+ }
Review Comment:
Same nit as above, I think we should just inline the boolean.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
