-
Notifications
You must be signed in to change notification settings - Fork 784
Conversation
The benchmark sounds interesting. @pwolfart did IMHO a great job testing database corruption of the mapdb. If you are interested, have a look at If your implementation is called to write very 499 ms it seems that the data is never written. It is a nice feature that you can manually edit the storage on corruption, but I assume this is a feature for us / developers only. This is nothing that a normal user wants to do. But IMHO we can merge such a storage backend (have not had a look at the code) and leave it to the product which one is chosen to use. |
Honestly, I would love to have such an (optional) storage system, especially during development but also while setting up a fresh system. I just love to see what makes a system work 😄 |
For me as very experienced power-user this feature is welcome. Being beta-tester from the first version of OH2 it was a nightmare to reconfigure over 100 of items after the database-corruption. |
Interesting corner case - I'll take a look. I would guess in reality this nearly never happens, but it would be good to cover.
No - I disagree. There have been a lot of discussions on the OH2 forum where people want to be able to edit the database while still having the ability to use the UI to configure the system. |
I added a maximum deferred timer of 60 seconds, so the data will be written every 60 seconds if there was a pathological instance of a continuous write. |
outputStream.write(s.getBytes()); | ||
outputStream.close(); | ||
} catch (Exception e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger?
I fully agree with Chris on this. I think it would be invaluable to a lot of power users and not just developers. |
Okay, perhaps it has been misspelled using "developers" and "users". I don't think such an user is willing to edit files on his system. As stated above, I think it is a nice feature and could be merged to the tree (IMHO). |
If the need is to have something that has higher integrity, then it would be relatively simple to add extra checks to improve the integrity by reading back from disk, and automatically reverting to the previous backup. |
When will this be added to OpenHAB2 :-) |
@@ -0,0 +1,202 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which component of this PR is licensed using the APL2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this ended up in the repo - I’ll remove it before this is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cdjackson, finally I took the time to review this! Looks good and indeed seems to work pretty well.
Just had some minor review comments, would be great if you could adapt the PR accordingly.
Please also don't miss to add this bundle to https://github.com/eclipse/smarthome/blob/master/bundles/storage/pom.xml, https://github.com/eclipse/smarthome/blob/master/features/org.eclipse.smarthome.feature.runtime.core/feature.xml#L176 and https://github.com/eclipse/smarthome/blob/master/features/karaf/src/main/feature/feature.xml#L297. Thanks!
* @author Thomas Eichstaedt-Engelen - Initial contribution | ||
* @author Alex Tugarev - Added test for getStorage without classloader | ||
*/ | ||
class StorageServiceOSGiTest extends OSGiTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you changed this class (from mapdb) only for testing and accidentially committed this file?
@@ -0,0 +1,136 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please update the license headers to the current version?
@@ -0,0 +1,129 @@ | |||
package org.eclipse.smarthome.storage.json; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a license header
|
||
private final Logger logger = LoggerFactory.getLogger(JsonStorage.class); | ||
|
||
private final int MAX_FILES = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make MAX_FILES, WRITE_DELAY and MAX_DEFERRED_PERIOD configuration properties of JsonStorageService, so that they can be set through ConfigAdmin?
private File getBackupFile(int age) { | ||
// Delete old backups | ||
List<Long> fileTimes = new ArrayList<Long>(); | ||
File folder = new File(file.getParent() + File.separator + "backup"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better define "backup" as a constant.
try { | ||
outputStream = new FileOutputStream(file, false); | ||
outputStream.write(s.getBytes()); | ||
outputStream.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either move this to finally or use try-with-resources
commitDatabase(); | ||
} | ||
if (deferredSince == 0) { | ||
deferredSince = System.currentTimeMillis(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use System.nanoTime()
public synchronized void deferredCommit() { | ||
// Handle a maximum time for deferring the commit. | ||
// This stops a pathological loop preventing saving | ||
if (deferredSince != 0 && deferredSince < System.currentTimeMillis() - MAX_DEFERRED_PERIOD) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use System.nanoTime()
@@ -0,0 +1,100 @@ | |||
package org.eclipse.smarthome.storage.mapdb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add license header
@@ -0,0 +1,33 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like an accidential commit?
9e3bc10
to
9ee4393
Compare
@kaikreuzer Hopefully this is updated to reflect all your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a few further review comments.
Once you are done, could you please squash your commits and add an
Also-By: IVAN GEORGIEV ILIEV ivan.iliev@musala.com
line to the commit message (as two classes are copied from him?)
I'll then have to create a CQ since we exceed 1000 loc.
* | ||
* @author Chris Jackson - Initial Contribution | ||
*/ | ||
public class JsonStorageService implements StorageService, ManagedService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not use ManageService - use DS component configuration through activate() instead.
} | ||
|
||
if (properties.get(CFG_MAX_BACKUP_FILES) != null) { | ||
maxBackupFiles = Integer.parseInt((String) properties.get(CFG_MAX_BACKUP_FILES)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer.parseInt can throw RuntimeExceptions, which you need to handle.
* Json bundle) is used in order to load the classes in the context of | ||
* the calling bundle. | ||
* | ||
* @param json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either add some text or remove those lines
<implementation class="org.eclipse.smarthome.storage.json.JsonStorageService" /> | ||
|
||
<property name="service.pid" type="String" value="org.eclipse.smarthome.storage.json" /> | ||
<property name="service.config.description.uri" type="String" value="system:json_storage" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find a config description for system:json_storage.
Anyhow, I don't think those settings need to be user adjustable through the UI - so I'd suggest to remove those service.config properties here.
@kaikreuzer regarding the config definition - I missed committing the file. However, I do think it's useful for people to be able to change these settings. For example, people using SSD/USB for storage may want to increase the delays to reduce the write cycles and avoid corruption. Sure, they could do it with config files, but doesn't it make more sense to have it configurable through the UI if config is available? |
ok for me. |
|
||
<config-description uri="system:json_storage"> | ||
<parameter name="backup_files" type="integer" min="0" max="100"> | ||
<label>Backup Files</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to "files" to be in sync with the other labels?
ce28420
to
5061265
Compare
@kaikreuzer this should be updated now I hope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, still not done...
immediate="true" name="org.eclipse.smarthome.storage.json"> | ||
<implementation class="org.eclipse.smarthome.storage.json.JsonStorageService" /> | ||
|
||
<property name="service.pid" type="String" value="org.eclipse.smarthome.storage.json" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you replace the spaces by tabs?
protected void deactivate(ComponentContext cContext, int reason) { | ||
} | ||
|
||
public synchronized void modified(Map<String, Object> properties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't specify the existence of the modified method in your component xml. And you shouldn't as your service is not able to deal with changes after it has been activated. Hence you should simply move this code in here to activate() and remove the modified() method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - interestingly, modified does get called ;).
|
||
private final Map<String, JsonStorage<Object>> storageList = new HashMap<String, JsonStorage<Object>>(); | ||
|
||
public void activate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have two activate methods now. You should move the code from here to the newly introduced one.
} | ||
|
||
public void deactivate() { | ||
// Since we're using a delayed commit, we need to write out any data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have two deactivate methods now. You should move the code from here to the newly introduced one.
@@ -1,13 +1,4 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems to be completely messed up - could you please check?
5061265
to
0b74172
Compare
Updated again... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, I think!
if (properties.get(CFG_MAX_DEFER_DELAY) != null) { | ||
maxDeferredPeriod = Integer.parseInt((String) properties.get(CFG_MAX_DEFER_DELAY)); | ||
} | ||
} catch (NumberFormatException nfe) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better do a catch for every single value - as it is now, you will ignore write_delay and defer_delay parameters, if backup_files cannot be parsed, even if they are set correctly.
maxDeferredPeriod = Integer.parseInt((String) properties.get(CFG_MAX_DEFER_DELAY)); | ||
} | ||
} catch (NumberFormatException nfe) { | ||
logger.error("NumberFormatException: {}", nfe.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better do a warning like: "Value xxx is invalid for parameter yyy - using zzz as a default instead"
Signed-off-by: Chris Jackson <chris@cd-jackson.com> Also-By: IVAN GEORGIEV ILIEV <ivan.iliev@musala.com>
0b74172
to
9ba4c66
Compare
Logging updated... |
Thanks for your patience :-) |
Created CQ 12251. |
CQ has been approved. |
@cdjackson Travis fails with a compilation error on the JSONStorage tests - could you please check? If it needs a fix, please add it as a new commit to your branch (do not squash!) - thanks! |
I've updated this to remove the benchmark 'test' which was causing the issue. I don't think it's a good idea to have it - certainly not as a test anyway. |
Thanks @cdjackson, but you forgot to sign-off your second commit! |
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
f402484
to
d98b79c
Compare
Sorry - updated... |
Perfect, thanks! |
Isn't it still the wrong license header? |
Closes openhab#202 (openhab#202) Sources of info: a) https://community.openhab.org/t/replacement-of-mapdb-by-jsondb/17498 b) eclipse-archived/smarthome#1868 Signed-off-by: Angelos <agf@itcs.ro> (github: AngelosF)
* Add content to jsondb.md Closes #202 (#202) Sources of info: a) https://community.openhab.org/t/replacement-of-mapdb-by-jsondb/17498 b) eclipse-archived/smarthome#1868 Signed-off-by: Angelos <agf@itcs.ro> (github: AngelosF) * Add minor change for testing purpose Signed-off-by: Thomas Dietrich <thomas.dietrich@tu-ilmenau.de> (github: ThomDietrich) * Adressed some review comments Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Changed to one sentence per line everywhere Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Improved backup file description Signed-off-by: Jerome Luckenbach <github@luckenba.ch> * Removed empty example file code fences. Signed-off-by: Confectrician <github@luckenba.ch>
This is an implementation of a json storage service for initial comment. Its main advantage is that it allows a user to edit the files with a text editor or json editor and is easily human readable. It is also considerably faster than mapdb. It has the potential to be less robust, but given it can be edited, and backups easily maintained, this is considered to be a low risk (currently!).
This implements the StorageService as a Map, but persists the data to disk as json files. Each storage that is opened is persisted to a separate file. Data is only read from the disk on startup.
The system employs a delayed write mechanism (currently 500ms) to improve performance where there are multiple writes in a short time. When the service is closed, all open services are written to disk so there should be no risk of loosing data unless the JVM crashes and the delay could be increased a little (probably still best to keep it low to reduce the chance of lost data due to a crashed JVM).
The service keeps backups of each table - currently up to 5 backups are maintained. Each time the json file is written, the old file is moved into a backup folder, and old files are removed from the backup folder.
Benchmark tests indicate that this is significantly faster than the mapdb storage service (20 to 30 times faster). With the pretty print option enabled the storage size is similar to the mapdb, and about 30% smaller if this is disabled. Currently I've left this enabled since the major advantage of this service is that the data can be edited easily so improving its readability at the expense of some disk space is considered acceptable.
Benchmarks