Design/Code Review Guidelines
Design Reviews Should Check For:
How-to design wiki for Sqoop2.
...
- Scope of this feature, what it does intend to change and what it does not.
- What the relevant components in Sqoop that will be affected by this change?
- It might be good to help them break down the design if it is too large and will extend more than few weeks to get the first cut.
- Ask them if any of the design decisions were based on some benchmarks/testing if performance is stated as a goal.
- Make sure they have done even research to leverage existing utilities/libraries instead of re-inventing the wheel again.
- Will there be any backwards incompatible changes?
- Will any dependencies be added?
Code Reviews Should Check For:
- Is the code change consistent with the proposed design? If there is a proposal to change the design, discuss the proposal on Jira.
- Is the new feature or bug fix well covered by unit tests?
- Is the new code sufficiently documented (i.e. code comments, restructured text, and wiki where needed) ? is facing
- aspects, the user documentation must be updated.
- If the feature has administrative facing aspects, the administration documentation should be updated.
- If the feature has developer facing aspects, the developer documentation should be updated.
- Does the code break backward compatibility? (For example, adding abstract methods to class used by connectors)
- Are there new dependencies? Are there new dependencies on non-open-source components?
- Is the placement of objects in packages consistent with the rest of the project?
- Does the patch apply? Unit and integration tests pass?
- If a new package/component is required, it should be highlighted in the design doc.
Does "mvn clean verify" pass?
- Is code duplicated?
- Sometimes take the extra step to give an example of what you might have done if it is few lines of code.
More in-depth review checklist per sqoop component.
Component | Must not miss checklist | How to test ? |
---|
API additions or changes - connector-sdk ( IDF API )
- sqoop-spi ( connector, configs, annotations ..)
- sqoop-core (repository API,
ExecutionEngine API ...) - sqoop-common ( Schema, Column API)
| - java docs and corresponding doc
- do not break backward compatibility unless there is major version update and it is explicitly discussed to do so
- when keeping the old and adding the new, make sure the old is deprecated
- consistent naming
- ask questions about why it is interface vs a abstract class vs annotation if not already stated
- consistent naming for attributes and methods
| |
RepositoryRespository - sqoop-repository-common
- sqoop-repository-derby
- sqoop-repository-x
| - Make sure the repo changes in Derby/XXXX are well documented, goes without saying to add comments to explain the SQL command nuances if need be for actual repository changes
- Make sure the right constraints for the new table/ field are defined as per the assumptionsfor actual repository changes
- Make sure to check if the repo version needs to change in this RB
- Make sure there is tests to cover the upgrades from all supported versions Unit tests are a must for every new API that is added.
to the new/current version - Tests should be named well to understand what it is testing
- Nice to encourage to have code-coverage report/stats for a new component.
| - mvn test
- mvn integration-test
- make sure sqoop-server starts ( since repository upgrade is called by default).
- Verify that the
sqoop.log and derbyrepo.log is clean - make sure to run upgrade tool and that is works.
|
Integration Test | | - Any reason these news test infra will add significant time to the test runs?
| |
Configurable Connectors and DriverConnectors ( inhouse) | - Make sure there is a upgrade path if the connector config structure changed
- Make sure to check validators are added for configs
- Tests as usual are a must for connector/driver changes
| - make sure sqoop-server starts
- Verify that the
sqoop.log and derbyrepo.log is clean - make sure to check that the
sqoop.properties has the connector.upgrade and driver.upgrade on and then start the server again - make sure to run upgrade tool and that is works.
|
MR Execution/Submission Engine - sqoop-execution-mapreduce
- sqoop-submission-mapreduce
- And future engines
| - Make sure to ask for perf tests for any core changes
- Make sure there is really thorough testing ( unit / integration ) added for this module since it is the core of everything.
| - mvn test && mvn integration-test
|
Sqoop Server | - The REST api changes should be reviewed for consistently and usability
- Make sure the corresponding docs are updated for any changes
- Review for breaking backward compatibility
| - mvn test && mvn integration-test
|
Sqoop CLI ( Shell) | - Make sure the changes to shell commands are documented
- Make sure the Shell commands are consistent
| - mvn test && mvn integration-test
|
Sqoop Client | - Make sure the changes to client APIs are documented.
- Make sure the Client APIs are consistent
| - mvn test && mvn integration-test
|
Security | - Make sure test with all the combinations of security configs if the security core has been modified
| - mvn test && mvn integration-test
|
Tools | - Make sure the tools actually work before signing off the review!
| - mvn test && mvn integration-test
|
Docs | - Cut some slack on this one! Make it easy on reviews so more docs are written ... as long as it is readable and has code examples
| - mvn site and verify the docs are readable
|