BsoBird commented on code in PR #9546:
URL: https://github.com/apache/iceberg/pull/9546#discussion_r1493324502


##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -355,71 +475,90 @@ int findVersion() {
    * an attempt will be made to delete the source file.
    *
    * @param fs the filesystem used for the rename
-   * @param src the source file
-   * @param dst the destination file
+   * @param tempMetaDataFile the source file
+   * @param finalMetaDataFile the destination file
    */
-  private void renameToFinal(FileSystem fs, Path src, Path dst, int 
nextVersion) {
+  @VisibleForTesting
+  boolean commitNewVersion(
+      FileSystem fs, Path tempMetaDataFile, Path finalMetaDataFile, Integer 
nextVersion)
+      throws IOException {
     try {
-      if (!lockManager.acquire(dst.toString(), src.toString())) {
+      if (!lockManager.acquire(finalMetaDataFile.toString(), 
finalMetaDataFile.toString())) {

Review Comment:
   According to the previous logic, this lock doesn't really work at all. So I 
did it on purpose.



##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -355,71 +475,90 @@ int findVersion() {
    * an attempt will be made to delete the source file.
    *
    * @param fs the filesystem used for the rename
-   * @param src the source file
-   * @param dst the destination file
+   * @param tempMetaDataFile the source file
+   * @param finalMetaDataFile the destination file
    */
-  private void renameToFinal(FileSystem fs, Path src, Path dst, int 
nextVersion) {
+  @VisibleForTesting
+  boolean commitNewVersion(
+      FileSystem fs, Path tempMetaDataFile, Path finalMetaDataFile, Integer 
nextVersion)
+      throws IOException {
     try {
-      if (!lockManager.acquire(dst.toString(), src.toString())) {
+      if (!lockManager.acquire(finalMetaDataFile.toString(), 
finalMetaDataFile.toString())) {

Review Comment:
   Whilst I don't think it makes sense to put the locks in this way, it really 
isn't relevant to the current PR and I will restore 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.

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

Reply via email to