You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 10 Next »

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

Code Reviews Should Check For:

  • Is the code change consistent with the proposed design?
  • If at this point some of the design decisions have to change, highlight them clearly and get back to discussing them on JIRA or design wiki, do not do back and forths in RBs. RB should be mostly for code review
  • Is the new feature or bug fix well covered by unit tests?
  • Is the new code sufficiently documented (i.e. code comments and wiki where needed) ? 
  • If the feature is user facing, the user documentation must 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 the patch apply? Unit and integration tests pass?
  • Learn to spot duplicate code patterns.!
  • 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 as long as it is readable and has code examples
  • mvn site and verify the docs are readable
  • No labels