[mapdb] Improve shutdown handling and add tests by holgerfriedrich · Pull Request #20153 · openhab/openhab-addons

holgerfriedrich · 2026-01-27T09:39:21Z

Track all storage requests and make sure we wait for them to complete before shutting down the database.
This might delay the DB shutdown up to 30 seconds. New storage requests issued during shutdown will cause a log entry (on info level).
Tests for the MapDb service.
Similar approach as in [rrd4j] Add unit tests #20142.
Includes code coverage metrics.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Copilot

Pull request overview

This pull request adds unit tests for the MapDB persistence service along with JaCoCo code coverage configuration. The tests follow a similar pattern to the referenced PR #20142 for RRD4J persistence, providing smoke tests for basic store and query operations.

Changes:

Added comprehensive unit tests for MapDbPersistenceService covering store/retrieve operations for different item types (Number, String, Switch)
Configured JaCoCo Maven plugin with code instrumentation for test coverage reporting
Added test coverage for service metadata methods (getId, getLabel)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
bundles/org.openhab.persistence.mapdb/src/test/java/org/openhab/persistence/mapdb/MapDbPersistenceServiceTest.java	New test class with 8 test methods covering persistence operations for different item types, query operations, and service metadata
bundles/org.openhab.persistence.mapdb/pom.xml	Added JaCoCo dependencies and Maven plugin configuration for code coverage reporting with offline instrumentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...rsistence.mapdb/src/test/java/org/openhab/persistence/mapdb/MapDbPersistenceServiceTest.java

holgerfriedrich · 2026-01-30T17:36:51Z

some valid comments from copilot, I will have a look

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Refs: openhab#20181 Signed-off-by: Holger Friedrich <mail@holger-friedrich.de> Co-authored-by: Copilot <Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...rsistence.mapdb/src/test/java/org/openhab/persistence/mapdb/MapDbPersistenceServiceTest.java

Copilot · 2026-02-06T22:19:55Z

...rsistence.mapdb/src/test/java/org/openhab/persistence/mapdb/MapDbPersistenceServiceTest.java

+            try {
+                Iterable<HistoricItem> results = service.query(criteria);
+                if (results.iterator().hasNext()) {
+                    long elapsed = System.currentTimeMillis() - startTime;
+                    logger.info("Storage completed for '{}' after {}ms ({} attempts)", itemName, elapsed, attempts);
+                    return; // Success!
+                }
+            } catch (Exception e) {
+                // Query might fail if data not ready yet, continue polling
+                logger.debug("Query attempt {} failed: {}", attempts, e.getMessage());


waitForStorage() catches any Exception from service.query() and keeps polling. For MapDbPersistenceService.query() an exception is unexpected (it typically indicates failed activation / null map) and swallowing it turns an immediate failure into a 20s timeout. Consider letting exceptions fail the test (or only retry on specific, expected transient errors).

Suggested change

try {

Iterable<HistoricItem> results = service.query(criteria);

if (results.iterator().hasNext()) {

long elapsed = System.currentTimeMillis() - startTime;

logger.info("Storage completed for '{}' after {}ms ({} attempts)", itemName, elapsed, attempts);

return; // Success!

}

} catch (Exception e) {

// Query might fail if data not ready yet, continue polling

logger.debug("Query attempt {} failed: {}", attempts, e.getMessage());

Iterable<HistoricItem> results = service.query(criteria);

if (results.iterator().hasNext()) {

long elapsed = System.currentTimeMillis() - startTime;

logger.info("Storage completed for '{}' after {}ms ({} attempts)", itemName, elapsed, attempts);

return; // Success!

Copilot · 2026-02-06T22:19:55Z

...rsistence.mapdb/src/test/java/org/openhab/persistence/mapdb/MapDbPersistenceServiceTest.java

+
+    @AfterEach
+    void tearDown() throws Exception {
+        if (service != null) {


waitForStorage() returns as soon as query() can read the value from the in-memory map, but store() persists asynchronously and only calls db.commit() after updating the map. This means the test may proceed (and tearDown() may close the DB) before the commit completes, leading to racy failures/corrupted storage. Consider adding a deterministic sync point (e.g., reopen the service and query again, or otherwise wait until the background store task has fully completed) before deactivating.

Suggested change

if (service != null) {

if (service != null) {

// Give asynchronous persistence enough time to complete before closing the DB

Thread.sleep(STORAGE_TIMEOUT_MS);

should be fine

...rsistence.mapdb/src/test/java/org/openhab/persistence/mapdb/MapDbPersistenceServiceTest.java

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

…before closing Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bundles/org.openhab.persistence.mapdb/pom.xml

Copilot · 2026-02-08T07:12:24Z

...rsistence.mapdb/src/test/java/org/openhab/persistence/mapdb/MapDbPersistenceServiceTest.java

+    @BeforeEach
+    void setUp() throws Exception {
+        // Create service and activate OSGi lifecycle manually for tests
+        service = new MapDbPersistenceService();
+        service.activate();
+    }


The tests reuse the same on-disk MapDB file between test cases (storage.mapdb under ${openhab.userdata}/persistence/mapdb). Since item names are reused across tests (e.g., TestNumber is used in multiple methods), a previous test run can satisfy waitForStorage() immediately and mask failures in the current test, making the suite order-dependent. Clean the MapDB storage directory/file in @BeforeEach (or use per-test unique item names / a per-test userdata folder).

using different item names for all tests runs now

Copilot · 2026-02-08T07:12:24Z

...ence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/MapDbPersistenceService.java

    public void activate() {
        logger.debug("MapDB persistence service is being activated");
+        active = true;

        try {
            Files.createDirectories(DB_DIR);


active is set to true at the start of activate(), but there are multiple early-return failure paths later in this method. That can leave the service in a partially-initialized state with active=true while db/map were never created, causing store() to proceed and then fail (potentially with NPE) instead of being skipped. Set active=true only after successful DB/map initialization, and ensure it is set back to false on all activation-failure returns.

i think this is fine as it is

...ence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/MapDbPersistenceService.java

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich · 2026-02-08T07:53:11Z

Ok, from my perspecitve, I am done. I have updated the PR description.

Can you please make sure that something mentioning the changed behaviour on close stays in the commit message when squashing during merge.

I am still not sure about the logging on info level if store() is called during shutdown, maybe it should be a debug.

lsiepel

Thanks, LGTM

[mapdb] Add unit tests

7d6bf71

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich requested a review from a team as a code owner January 27, 2026 09:39

holgerfriedrich added the test label Jan 27, 2026

lsiepel requested a review from Copilot January 30, 2026 16:39

Copilot started reviewing on behalf of lsiepel January 30, 2026 16:39 View session

Copilot AI reviewed Jan 30, 2026

View reviewed changes

holgerfriedrich and others added 2 commits February 6, 2026 21:57

review comments

bef4391

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Carry over adaptive polling from rrd4j persistence.

1d61a1e

Refs: openhab#20181 Signed-off-by: Holger Friedrich <mail@holger-friedrich.de> Co-authored-by: Copilot <Copilot@users.noreply.github.com>

holgerfriedrich requested a review from Copilot February 6, 2026 22:09

Copilot started reviewing on behalf of holgerfriedrich February 6, 2026 22:10 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

holgerfriedrich added 4 commits February 6, 2026 23:36

review comments

fe67d42

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Test if values are persisted to disk

3f91c87

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Add open task tracking to make sure all values are persisted to disk …

ccca810

…before closing Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Cleanup, simplify task tracking

b809127

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich requested a review from Copilot February 8, 2026 07:02

Copilot started reviewing on behalf of holgerfriedrich February 8, 2026 07:02 View session

holgerfriedrich changed the title ~~[mapdb] Add unit tests~~ [mapdb] Improve shutdown handling and add tests Feb 8, 2026

holgerfriedrich added the enhancement An enhancement or new feature for an existing add-on label Feb 8, 2026

Tweak logging

923f590

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

Copilot AI reviewed Feb 8, 2026

View reviewed changes

review comments

e9b8ba5

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

holgerfriedrich requested a review from lsiepel February 11, 2026 10:51

lsiepel approved these changes Feb 14, 2026

View reviewed changes

lsiepel merged commit 6b541b0 into openhab:main Feb 14, 2026
2 checks passed

lsiepel added this to the 5.2 milestone Feb 14, 2026

holgerfriedrich deleted the pr-mapdb-tests branch February 14, 2026 13:34

Uh oh!

Conversation

holgerfriedrich commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

holgerfriedrich commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

holgerfriedrich Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

holgerfriedrich Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

holgerfriedrich Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

holgerfriedrich commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsiepel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

holgerfriedrich commented Jan 27, 2026 •

edited

Loading

holgerfriedrich commented Jan 30, 2026 •

edited

Loading

holgerfriedrich commented Feb 8, 2026 •

edited

Loading