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
