Skip to content

dev.guidelines

Jump to bottom
Louis Jacomet edited this page Sep 13, 2016 · 6 revisions

Developer guidelines

These are some things to consider when you add code to Ehcache, but certainly when you review a PR!

General guidelines

Whenever you start working on an issue, you probably want to consider some, if not all, of the items below:

API changes

If you need to change the public API (this includes additions), you probably want to start with that first. An easy way to go about that, is to add or modify GettingStarted documentation to concretely expose how the API would be affected. You can do this on your branch, as a first commit, and already share it for review with a larger audience, e.g. on the mailing list or through a preliminary pull request.

Design

Whether API is modified or not, you’ll want to consider how you plan to go about implementing it all. There are a couple of specificities of the Ehcache internals that would need to be accounted for:

  1. Service additions or modifications: this is mainly about IoC and LSP, we want subsystems of your feature to be both injected and abstracted away behind an interface. This allows the actual implementation to be easily replaced with another. We also try to reuse existing Service definitions as much as possible. So don’t hesitate to slightly tweak an existing one, rather than introducing a new one.

  2. Lifecycle: You’ll want to assess how Ehcache’s life cycling may affect your change (if at all). If anything requires any special life cycling (e.g. PersistentCacheManager services), you will have to account for this.

  3. Failure: Most things eventually do go wrong. Whether it’s IO, some user provided implementation throwing, the actual Store being unaccessible…​ things just go wrong. Ehcache tries to never do any harm to the user’s application. As such your change should account for failure scenarios and possibly delegate to the ResilienceStrategy to enable users to deal with such scenarios sensibly.

Last touches

Once your change is implemented (see coding guidelines below), you will want to make sure that:

  1. You added adequate (but not excessive) logging;

  2. Configuration is complete:

    1. XML configuration was added, but that should never drive the actual configuration concerns;

    2. XML Templating is correctly supported.

Coding guidelines

Commit comment

  • Make sure the issue the commit relates is referenced in the commit comment.

    • If your commit closes an issue, think about using the GitHub options for automatically closing the issue when the commit reaches master.

  • For your commit comment, you are strongly encouraged to follow the recommendations found here

    • Feel free also to use emoji - current ones used are based on the following

Here is an example:

:bug: Fix #1354 JSR-107 support in ClusteredStore stats

Some context registrations are required to get the proper statistics
discovered by the JCache stats MBean

Formatting

See what’s there for now…​ Ping us on the developer mailing list. We’ll export settings for majors IDEs here asap.

Unit testing

All changes need to come with appropriate test coverage.

Adding code

All this new code you add should have test coverage.

Breaking existing tests

Should you break an existing test, take great care before touching the test code to "fix it". While it could definitively be that the test is wrong, it may be correct as well. Always assume the latter is true first. If in doubt, again, ping us on the developer mailing list.

Changing an existing test for it to pass, because of you recent changes, should really be the last resort. Other than for compilation obviously, or adding mocks and the like.

SPI tests

We have an SPI module that covers testing functionality that needs to be provided by a given SPI, e.g. org.ehcache.spi.cache.Store.

Warning
 work in progress

Logging

We use slf4j for all our logging here…​

Log levels

What level should be used for logging should be dependent on whether you log for the user’s benefit (e.g. lifecycle), debugging purposes (a standard Cache.get(K): V path). But we don’t log WARN or ERROR.

At INFO level

We log things about lifecycle:

  • Bootstrapping CacheManager,

  • Adding, removing Cache to CacheManager

  • …​

Basically everything that would be informational to the end-user.

Level DEBUG and below

Helps us trace stuff, when some weird scenario is being debugged.

  • Delegating to CacheLoader,

  • Delegating to ResilienceStrategy,

  • Store internals…​

Levels WARN & ERROR

Nothing really, if that level is required, we would rather want to throw!

Javadoc

Public types

Needs to be fully Javadoc’ed

Internal concrete classes

Require at least class-level Javadoc. But we value clear method, arguments and variable names above all here.

Clone this wiki locally