Github user steveloughran commented on the pull request: https://github.com/apache/spark/pull/5423#issuecomment-116667971 Thanks for sitting down to review it; it has grown to handle the end to end problem, auth, unreliable endpoints, etc, where some complexity is always the curse. 1. Note that most of the code is actually in the tests, a combination of mock, unit and functional -in the latter there's stuff that I hadn't seen the spark codebase yet, spinning up web services in VM and working with them. So yes, they do add more code, though some of that could be factored up into base modules. I just kept things as isolated into one path as possible. 2. Splitting up the patch? Would that help? Without that history-server side of things, there's not much in the way of testing the publishing aspects âespecially of the big one "can the published events be unmarshalled and used to rebuld AppUI instances. If you want it split up for reviewing, I'll gladly do it, with that caveat that full test coverage comes when the History server joins in. 3. Style things, easily addressed âthe usual subtleties of different project's expectations. Regarding app attempts, this pull request has been up for review since before they came out; it's been chasing a moving target. I've also been keeping the code working against 1.3, the testing of which kept the scale and security coverage up. I hadn't done the app attempt stuff yet because of the high rate of change there, and was reasonably confident that once checked in another iteration would round it off. Now that works done I have the time to finish of these details -but its still trying to track something relatively unstable, so needs to get in. How about, then 1. I do all the style comments & tag test only methods as `@VisibleForTesting`. If there are some other things you've not commented on, please highlight them. 2. I'll sync it up with the current payload of messages Unless it really makes a fundamental difference in getting the patch reviewed, I'd like to keep things together on the grounds of testability. But if splitting it up is what it takes to get in, that's what I'll do.
--- 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