marchpure opened a new pull request #3886: URL: https://github.com/apache/carbondata/pull/3886 … happen ### Why is this PR needed? In the insertstage flow, the stage files will be deleted with retry mechanism. but then IOException happen due to network abnormal etc, the delete stage flow will be interrupted, which is unexpected. ### What changes were proposed in this PR? When catch exception during deleting stages files, Continue to retry. ### Does this PR introduce any user interface change? - No ### Is any new testcase added? - No ---------------------------------------------------------------- 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: [hidden email] |
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-670501876 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3654/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-670504444 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1915/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-670930533 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3662/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-670931424 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1923/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
Indhumathi27 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671268912 retest this please ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671332155 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1939/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671334639 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3678/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
Indhumathi27 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671410491 LGTM ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
xubo245 commented on a change in pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#discussion_r468283023 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertFromStageCommand.scala ########## @@ -557,25 +564,30 @@ case class CarbonInsertFromStageCommand( * Return the files failed to delete */ private def deleteStageFiles( + stagePath: String, executorService: ExecutorService, stageFiles: Array[(CarbonFile, CarbonFile)]): Array[(CarbonFile, CarbonFile)] = { stageFiles.map { files => executorService.submit(new Callable[(CarbonFile, CarbonFile, Boolean)] { override def call(): (CarbonFile, CarbonFile, Boolean) = { // Delete three types of file: stage|.success|.loading - val stageLoadingFile = - FileFactory.getCarbonFile(files._1.getAbsolutePath - + CarbonTablePath.LOADING_FILE_SUFFIX); - var isFailed = false - // If delete() return false, maybe the reason is FileNotFount or FileFailedClean. - // Considering FileNotFound means file clean successfully. - // We need double check the file exists or not when delete() return false. - if (!(files._1.delete() && files._2.delete() && stageLoadingFile.delete())) { - // If the file still exists, make isFailed to be true - // So we can retry to delete this file. - isFailed = files._1.exists() || files._1.exists() || stageLoadingFile.exists() + try { + val stageLoadingFile = FileFactory.getCarbonFile(stagePath + + CarbonCommonConstants.FILE_SEPARATOR + Review comment: Suggestion: use File.separator instead of CarbonCommonConstants.FILE_SEPARATOR, File.separator can support linux and window。 ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
xubo245 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671680064 Please optimize the PR title. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
xubo245 commented on a change in pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#discussion_r468283508 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertFromStageCommand.scala ########## @@ -499,25 +499,31 @@ case class CarbonInsertFromStageCommand( * return the loading files failed to create */ private def createStageLoadingFiles( + stagePath: String, executorService: ExecutorService, stageFiles: Array[(CarbonFile, CarbonFile)]): Array[(CarbonFile, CarbonFile)] = { stageFiles.map { files => executorService.submit(new Callable[(CarbonFile, CarbonFile, Boolean)] { override def call(): (CarbonFile, CarbonFile, Boolean) = { - // Get the loading files path - val stageLoadingFile = - FileFactory.getCarbonFile(files._1.getAbsolutePath + - CarbonTablePath.LOADING_FILE_SUFFIX); - // Try to create loading files - // make isFailed to be true if createNewFile return false. - // the reason can be file exists or exceptions. - var isFailed = !stageLoadingFile.createNewFile() - // if file exists, modify the lastModifiedTime of the file. - if (isFailed) { - // make isFailed to be true if setLastModifiedTime return false. - isFailed = !stageLoadingFile.setLastModifiedTime(System.currentTimeMillis()); + try { + // Get the loading files path + val stageLoadingFile = + FileFactory.getCarbonFile(stagePath + + CarbonCommonConstants.FILE_SEPARATOR + + files._1.getName + CarbonTablePath.LOADING_FILE_SUFFIX); + // Try to create loading files + // make isFailed to be true if createNewFile return false. + // the reason can be file exists or exceptions. + var isFailed = !stageLoadingFile.createNewFile() + // if file exists, modify the lastmodifiedtime of the file. + if (isFailed) { + // make isFailed to be true if setLastModifiedTime return false. + isFailed = !stageLoadingFile.setLastModifiedTime(System.currentTimeMillis()); + } + (files._1, files._2, isFailed) + } catch { + case _ : Exception => (files._1, files._2, true) Review comment: Why return (files._1, files._2, true) when exception happen ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertFromStageCommand.scala ########## @@ -499,25 +499,31 @@ case class CarbonInsertFromStageCommand( * return the loading files failed to create */ private def createStageLoadingFiles( + stagePath: String, executorService: ExecutorService, stageFiles: Array[(CarbonFile, CarbonFile)]): Array[(CarbonFile, CarbonFile)] = { stageFiles.map { files => executorService.submit(new Callable[(CarbonFile, CarbonFile, Boolean)] { override def call(): (CarbonFile, CarbonFile, Boolean) = { - // Get the loading files path - val stageLoadingFile = - FileFactory.getCarbonFile(files._1.getAbsolutePath + - CarbonTablePath.LOADING_FILE_SUFFIX); - // Try to create loading files - // make isFailed to be true if createNewFile return false. - // the reason can be file exists or exceptions. - var isFailed = !stageLoadingFile.createNewFile() - // if file exists, modify the lastModifiedTime of the file. - if (isFailed) { - // make isFailed to be true if setLastModifiedTime return false. - isFailed = !stageLoadingFile.setLastModifiedTime(System.currentTimeMillis()); + try { + // Get the loading files path + val stageLoadingFile = + FileFactory.getCarbonFile(stagePath + + CarbonCommonConstants.FILE_SEPARATOR + + files._1.getName + CarbonTablePath.LOADING_FILE_SUFFIX); + // Try to create loading files + // make isFailed to be true if createNewFile return false. + // the reason can be file exists or exceptions. + var isFailed = !stageLoadingFile.createNewFile() + // if file exists, modify the lastmodifiedtime of the file. + if (isFailed) { + // make isFailed to be true if setLastModifiedTime return false. + isFailed = !stageLoadingFile.setLastModifiedTime(System.currentTimeMillis()); + } + (files._1, files._2, isFailed) + } catch { + case _ : Exception => (files._1, files._2, true) Review comment: Why return (files._1, files._2, true) when exception 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: [hidden email] |
In reply to this post by GitBox
xubo245 commented on a change in pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#discussion_r468283733 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertFromStageCommand.scala ########## @@ -499,25 +499,31 @@ case class CarbonInsertFromStageCommand( * return the loading files failed to create */ private def createStageLoadingFiles( + stagePath: String, executorService: ExecutorService, stageFiles: Array[(CarbonFile, CarbonFile)]): Array[(CarbonFile, CarbonFile)] = { stageFiles.map { files => executorService.submit(new Callable[(CarbonFile, CarbonFile, Boolean)] { override def call(): (CarbonFile, CarbonFile, Boolean) = { - // Get the loading files path - val stageLoadingFile = - FileFactory.getCarbonFile(files._1.getAbsolutePath + - CarbonTablePath.LOADING_FILE_SUFFIX); - // Try to create loading files - // make isFailed to be true if createNewFile return false. - // the reason can be file exists or exceptions. - var isFailed = !stageLoadingFile.createNewFile() - // if file exists, modify the lastModifiedTime of the file. - if (isFailed) { - // make isFailed to be true if setLastModifiedTime return false. - isFailed = !stageLoadingFile.setLastModifiedTime(System.currentTimeMillis()); + try { + // Get the loading files path + val stageLoadingFile = + FileFactory.getCarbonFile(stagePath + + CarbonCommonConstants.FILE_SEPARATOR + Review comment: Suggestion: use File.separator instead of CarbonCommonConstants.FILE_SEPARATOR, File.separator can support linux and window。 ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
xubo245 commented on a change in pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#discussion_r468283782 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertFromStageCommand.scala ########## @@ -557,25 +564,30 @@ case class CarbonInsertFromStageCommand( * Return the files failed to delete */ private def deleteStageFiles( + stagePath: String, executorService: ExecutorService, stageFiles: Array[(CarbonFile, CarbonFile)]): Array[(CarbonFile, CarbonFile)] = { stageFiles.map { files => executorService.submit(new Callable[(CarbonFile, CarbonFile, Boolean)] { override def call(): (CarbonFile, CarbonFile, Boolean) = { // Delete three types of file: stage|.success|.loading - val stageLoadingFile = - FileFactory.getCarbonFile(files._1.getAbsolutePath - + CarbonTablePath.LOADING_FILE_SUFFIX); - var isFailed = false - // If delete() return false, maybe the reason is FileNotFount or FileFailedClean. - // Considering FileNotFound means file clean successfully. - // We need double check the file exists or not when delete() return false. - if (!(files._1.delete() && files._2.delete() && stageLoadingFile.delete())) { - // If the file still exists, make isFailed to be true - // So we can retry to delete this file. - isFailed = files._1.exists() || files._1.exists() || stageLoadingFile.exists() + try { + val stageLoadingFile = FileFactory.getCarbonFile(stagePath + + CarbonCommonConstants.FILE_SEPARATOR + + files._1.getName + CarbonTablePath.LOADING_FILE_SUFFIX); + var isFailed = false + // If delete() return false, maybe the reason is FileNotFount or FileFailedClean. + // Considering FileNotFound means FileCleanSucessfully. + // We need double check the file exists or not when delete() return false. + if (!files._1.delete() || !files._2.delete() || !stageLoadingFile.delete()) { + // If the file still exists, make isFailed to be true + // So we can retry to delete this file. + isFailed = files._1.exists() || files._1.exists() || stageLoadingFile.exists() + } + (files._1, files._2, isFailed) + } catch { + case _: Exception => (files._1, files._2, true) Review comment: Why return (files._1, files._2, true) when exception 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: [hidden email] |
In reply to this post by GitBox
xubo245 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671681041 reviewed, please check it. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
xubo245 edited a comment on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671680064 Please optimize the PR title and description(insertstage). ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
marchpure commented on a change in pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#discussion_r468453680 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertFromStageCommand.scala ########## @@ -557,25 +564,30 @@ case class CarbonInsertFromStageCommand( * Return the files failed to delete */ private def deleteStageFiles( + stagePath: String, executorService: ExecutorService, stageFiles: Array[(CarbonFile, CarbonFile)]): Array[(CarbonFile, CarbonFile)] = { stageFiles.map { files => executorService.submit(new Callable[(CarbonFile, CarbonFile, Boolean)] { override def call(): (CarbonFile, CarbonFile, Boolean) = { // Delete three types of file: stage|.success|.loading - val stageLoadingFile = - FileFactory.getCarbonFile(files._1.getAbsolutePath - + CarbonTablePath.LOADING_FILE_SUFFIX); - var isFailed = false - // If delete() return false, maybe the reason is FileNotFount or FileFailedClean. - // Considering FileNotFound means file clean successfully. - // We need double check the file exists or not when delete() return false. - if (!(files._1.delete() && files._2.delete() && stageLoadingFile.delete())) { - // If the file still exists, make isFailed to be true - // So we can retry to delete this file. - isFailed = files._1.exists() || files._1.exists() || stageLoadingFile.exists() + try { + val stageLoadingFile = FileFactory.getCarbonFile(stagePath + + CarbonCommonConstants.FILE_SEPARATOR + Review comment: modified ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
marchpure commented on a change in pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#discussion_r468453764 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertFromStageCommand.scala ########## @@ -499,25 +499,31 @@ case class CarbonInsertFromStageCommand( * return the loading files failed to create */ private def createStageLoadingFiles( + stagePath: String, executorService: ExecutorService, stageFiles: Array[(CarbonFile, CarbonFile)]): Array[(CarbonFile, CarbonFile)] = { stageFiles.map { files => executorService.submit(new Callable[(CarbonFile, CarbonFile, Boolean)] { override def call(): (CarbonFile, CarbonFile, Boolean) = { - // Get the loading files path - val stageLoadingFile = - FileFactory.getCarbonFile(files._1.getAbsolutePath + - CarbonTablePath.LOADING_FILE_SUFFIX); - // Try to create loading files - // make isFailed to be true if createNewFile return false. - // the reason can be file exists or exceptions. - var isFailed = !stageLoadingFile.createNewFile() - // if file exists, modify the lastModifiedTime of the file. - if (isFailed) { - // make isFailed to be true if setLastModifiedTime return false. - isFailed = !stageLoadingFile.setLastModifiedTime(System.currentTimeMillis()); + try { + // Get the loading files path + val stageLoadingFile = + FileFactory.getCarbonFile(stagePath + + CarbonCommonConstants.FILE_SEPARATOR + Review comment: modified ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671844968 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1955/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #3886: URL: https://github.com/apache/carbondata/pull/3886#issuecomment-671845557 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3694/ ---------------------------------------------------------------- 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: [hidden email] |
Free forum by Nabble | Edit this page |