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 

Code Reviews Should Check For:

 

 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