[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile
tgravescs commented on pull request #31496: URL: https://github.com/apache/spark/pull/31496#issuecomment-781404497 can we please rename pr and Jira and update the description. It would be good for jira to have description 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
[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile
tgravescs commented on pull request #31496: URL: https://github.com/apache/spark/pull/31496#issuecomment-775643486 The API as you state is Evolving and that is on purpose so we can extend and change as people use it and we learn more. I'm happy to hear the feedback and improve the API during normally development periods, at this point when we already had a 3.1 rc, it is not appropriate to throw API changes in last minute, most of them were discussed and chosen for a reason. This just leads to introducing more issues. I asked repeatedly for people to help review the SPIP and the code along the way - that was the time to help with API changes not right before a release goes out. I think this has happened way to much and I believe even brought up about the 3.0.0 release. That said, non-api changes or obvious small issues in it should be fixed. I'm fine with 1. and I'm fine with 2 for: Seq -> Array: https://github.com/apache/spark/pull/31496/files#diff-319a3f0dfd7de6045eb11ad960180230c47e6f52b1b27d3c9a1f2d72f1615d9dR284 build -> build() https://github.com/apache/spark/pull/31496/files#diff-a6d96a65d9905b310451b125acac6610ffbd6b4548461bd1d5a18dc29282814aR71 The third point api's were intentionally added so I'm against that change at this point. It does look like I forgot to add automated tests for it, we can file a jira and I will add them. We shouldn't remove an api because tests were missed. This is user facing api so the fact its not used internally doesn't mean its not useful. You can argue whether a builder should have a clear/remove api, but it really depends on how users use this set of API's. I have seen many builders with remove apis. 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
[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile
tgravescs commented on pull request #31496: URL: https://github.com/apache/spark/pull/31496#issuecomment-775276262 I'd really like to express my concern over this PR. There is a reason we go through a SPIP process and review the API's during design and PRs. Many things were discussed, sometimes compromises were made based on feedback, some things may be to allow supporting future enhancements, etc. I really hope this isn't happening in other places in the code. 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
[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile
tgravescs commented on pull request #31496: URL: https://github.com/apache/spark/pull/31496#issuecomment-775257493 I'm -1 for any changes here except maybe docs... it is way to late in the release to be changing api's. This was reviewed and went in. We have already put out an RC and this is not a blocking bug. I don't want last minute changes that were thought about to mess things up. 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
