holdenk commented on pull request #29788: URL: https://github.com/apache/spark/pull/29788#issuecomment-695530700
Since there was another PR in the same area committed that broke the existing integration tests in this area I don't feel confident with my soft reservations and switching to a vetoing for this change (e.g. -1). Technical justification: Lack of test coverage & I don't believe the stated benefit makes up for the increased risk. The suggested path forward: Mark PR as WIP, improve test coverage (ideally in a way which demonstrates the benefits), go through review process. I would like to suggest since this PR states that the reason this change is needed is to make the mechanism more extensible we should consider if a private sealed trait is well suited to achieving that goal. I believe that this part of the code may need to be revisited as well. ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
