Design/Code Review Guidelines

Design Reviews Should Check For:

 

How-to design wiki for Sqoop2.

Use the above as a guideline to ask specific questions about the design such as the 

  • 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) ? 
    • If the feature has user 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? 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.

ComponentMust not miss checklistHow 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
  • ask questions about why it is interface vs a abstract class vs annotation if not already stated
  • consistent naming for attributes and methods
  • mvn test

Repository

  • 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 for 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 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

  • test
  • common-test
  • Any reason these news test infra will add significant time to the test runs?

  • mvn integration-test

Configurable

Connectors and Driver

  • GenericJDBC
  • HDFS
  • Kite
  • XXXX


  • 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

  • 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)

  • sqoop-shell
  • Make sure the changes to shell commands are documented
  • Make sure the Shell commands are consistent
  • mvn test && mvn integration-test

Sqoop Client

  • sqoop-client
  • Make sure the changes to client APIs are documented.
  • Make sure the Client APIs are consistent
  • mvn test && mvn integration-test

Security

  • sqoop-security
  • Make sure test with all the combinations of security configs if the security core has been modified
  • mvn test && mvn integration-test

Tools

  • sqoop-tools
  • Make sure the tools actually work before signing off the review!
  • mvn test && mvn integration-test

Docs

  • sqoop-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
  • No labels