Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/4435#issuecomment-93898160
I'd like to finish reviewing this, but I keep getting pre-empted by other
work, so instead I'll leave a list of things that I would look at / check when
reviewing this (to let other folks pick up and finish the review). This looks
like it's in pretty good shape overall, though, so hopefully it won't be too
much work to finish this.
Here's what I'd look at in any final review passes:
- Has the visibility of new classes / methods / interfaces been restricted
to the narrowest possible scope (i.e. are we unintentionally exposing internal
functionality)? If something _has_ to be public but is not intended to be
stable / available to users, we should add a documentation comment to explain
this.
- Have accesses to listeners been properly synchronized?
- Are there any code style nits that we should clean up? I noticed a bunch
of minor indentation problems, but don't really have time to comment
individually.
- I'd take a look at how we handle timestamps in JSON, just to double-check
that we're exposing them in an easy-to-consume format.
- Documentation-wise, are there any confusing parts of the code that need
to be documented?
- Can we add a top-level Javadoc comment somewhere to explain our overall
strategy for handling JSON compatibility, etc, and maybe a checklist / rules to
follow when changing these classes? There's something similar to this in one
of the JSONProtocol classes, which might be nice to model this on.
I'd also manually test this in a spark-shell.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
