Github user ssimeonov commented on a diff in the pull request: https://github.com/apache/spark/pull/16204#discussion_r91437338 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatWriter.scala --- @@ -225,32 +228,50 @@ object FileFormatWriter extends Logging { taskAttemptContext: TaskAttemptContext, committer: FileCommitProtocol) extends ExecuteWriteTask { - private[this] var outputWriter: OutputWriter = { + private[this] var currentWriter: OutputWriter = _ --- End diff -- Looking through the code, three things stand out: 1. There is code duplication between `SingleDirectoryWriteTask` and `DynamicPartitionWriteTask` when it comes to current writer management and cleanup. 2. There is duplication within `releaseResources()` and `newOutputWriter()` of the write tasks when it comes to releasing resources. 3. Write task state management is leaky because `releaseResources()` is called explicitly by `executeTask()`. Also, `releaseResources()` will be called twice when there are no exceptions and once if there is an exception in `execute()`, which is a bit confusing. What about asking the base trait to do a bit more work and present a stronger contract to its users, e.g.: ```scala private trait ExecuteWriteTask { protected[this] var currentWriter: OutputWriter = null def execute(iterator: Iterator[InternalRow]): Set[String] = { try { executeImp(iterator) } finally { releaseResources() } } /** * Writes data out to files, and then returns the list of partition strings written out. * The list of partitions is sent back to the driver and used to update the catalog. */ protected def executeImp(iterator: Iterator[InternalRow]): Set[String] protected def resetCurrentWriter(): Unit = { if (currentWriter != null) { currentWriter.close() currentWriter = null } } protected def releaseResources(): Unit = { resetCurrentWriter() } } ```
--- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org