From a0ca87530991900db5b34df3683aa010dffe97c8 Mon Sep 17 00:00:00 2001 From: Michael Mauersberger <67641254+m-mauersberger@users.noreply.github.com> Date: Sat, 26 Sep 2020 07:39:16 +0200 Subject: [PATCH] Fixes Shared Database: Changes filtering in CoarseChangeFilter to attribute property (#6868) --- docs/advanced-reading/remote-storage.md | 20 +++++++ .../autosaveandbackup/AutosaveManager.java | 6 ++ .../autosaveandbackup/BackupManager.java | 4 +- .../jabref/logic/shared/DBMSSynchronizer.java | 48 +++++++++++++--- .../jabref/logic/util/CoarseChangeFilter.java | 55 ++++++++----------- .../jabref/logic/util/DelayTaskThrottler.java | 18 +++++- .../event/BibDatabaseContextChangedEvent.java | 20 ++++++- .../model/entry/event/EntriesEvent.java | 1 + .../model/entry/event/FieldChangedEvent.java | 6 +- .../metadata/event/MetaDataChangedEvent.java | 1 + 10 files changed, 131 insertions(+), 48 deletions(-) create mode 100644 docs/advanced-reading/remote-storage.md diff --git a/docs/advanced-reading/remote-storage.md b/docs/advanced-reading/remote-storage.md new file mode 100644 index 00000000000..9fe71744a10 --- /dev/null +++ b/docs/advanced-reading/remote-storage.md @@ -0,0 +1,20 @@ +# Remote Storage + +## Using a shared PostgreSQL database + +... + +## Handling large shared databases + +Synchronization times may get long when working with a large database containing several thousand entries. Therefore, synchronization only happens if several conditions are fulfilled: + +* Edit to another field. +* Major changes have been made (pasting or deleting more than one character). + +Class `org.jabref.logic.util.CoarseChangeFilter.java` checks both conditions. + +Remaining changes that has not been synchronized yet are saved at closing the database rendering additional closing time. Saving is realized in `org.jabref.logic.shared.DBMSSynchronizer.java`. Following methods account for synchronization modes: + +* `pullChanges` synchronizes the database unconditionally. +* `pullLastEntryChanges` synchronizes only if there are remaining entry changes. It is invoked when closing the shared database (`closeSharedDatabase`). + diff --git a/src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java b/src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java index 1ba127033d1..8a14f7fc533 100644 --- a/src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java +++ b/src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java @@ -42,6 +42,12 @@ private AutosaveManager(BibDatabaseContext bibDatabaseContext) { @Subscribe public synchronized void listen(@SuppressWarnings("unused") BibDatabaseContextChangedEvent event) { + if (!event.isFilteredOut()) { + startAutosaveTask(); + } + } + + private void startAutosaveTask() { throttler.schedule(() -> { eventBus.post(new AutosaveEvent()); }); diff --git a/src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java b/src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java index 36d1b0a1f53..60cccdd855a 100644 --- a/src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java +++ b/src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java @@ -156,7 +156,9 @@ private void logIfCritical(Path backupPath, IOException e) { @Subscribe public synchronized void listen(@SuppressWarnings("unused") BibDatabaseContextChangedEvent event) { - startBackupTask(); + if (!event.isFilteredOut()) { + startBackupTask(); + } } private void startBackupTask() { diff --git a/src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java b/src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java index 9aefa1049bb..e4b7e0294c3 100644 --- a/src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java +++ b/src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java @@ -54,6 +54,7 @@ public class DBMSSynchronizer implements DatabaseSynchronizer { private final Character keywordSeparator; private final GlobalCitationKeyPattern globalCiteKeyPattern; private final FileUpdateMonitor fileMonitor; + private Optional lastEntryChanged; public DBMSSynchronizer(BibDatabaseContext bibDatabaseContext, Character keywordSeparator, GlobalCitationKeyPattern globalCiteKeyPattern, FileUpdateMonitor fileMonitor) { @@ -64,6 +65,7 @@ public DBMSSynchronizer(BibDatabaseContext bibDatabaseContext, Character keyword this.eventBus = new EventBus(); this.keywordSeparator = keywordSeparator; this.globalCiteKeyPattern = Objects.requireNonNull(globalCiteKeyPattern); + this.lastEntryChanged = Optional.empty(); } /** @@ -77,10 +79,13 @@ public void listen(EntriesAddedEvent event) { // In this case DBSynchronizer should not try to insert the bibEntry entry again (but it would not harm). if (isEventSourceAccepted(event) && checkCurrentConnection()) { synchronizeLocalMetaData(); - synchronizeLocalDatabase(); // Pull changes for the case that there were some + pullWithLastEntry(); + synchronizeLocalDatabase(); dbmsProcessor.insertEntries(event.getBibEntries()); - } + // Reset last changed entry because it just has already been synchronized -> Why necessary? + lastEntryChanged = Optional.empty(); } + } /** * Listening method. Updates an existing shared {@link BibEntry}. @@ -89,13 +94,17 @@ public void listen(EntriesAddedEvent event) { */ @Subscribe public void listen(FieldChangedEvent event) { + BibEntry bibEntry = event.getBibEntry(); // While synchronizing the local database (see synchronizeLocalDatabase() below), some EntriesEvents may be posted. // In this case DBSynchronizer should not try to update the bibEntry entry again (but it would not harm). - if (isPresentLocalBibEntry(event.getBibEntry()) && isEventSourceAccepted(event) && checkCurrentConnection()) { + if (isPresentLocalBibEntry(bibEntry) && isEventSourceAccepted(event) && checkCurrentConnection() && !event.isFilteredOut()) { synchronizeLocalMetaData(); - BibEntry bibEntry = event.getBibEntry(); + pullWithLastEntry(); synchronizeSharedEntry(bibEntry); synchronizeLocalDatabase(); // Pull changes for the case that there were some + } else { + // Set new BibEntry that has been changed last + lastEntryChanged = Optional.of(bibEntry); } } @@ -110,10 +119,10 @@ public void listen(EntriesRemovedEvent event) { // While synchronizing the local database (see synchronizeLocalDatabase() below), some EntriesEvents may be posted. // In this case DBSynchronizer should not try to delete the bibEntry entry again (but it would not harm). if (isEventSourceAccepted(event) && checkCurrentConnection()) { - List entries = event.getBibEntries(); - dbmsProcessor.removeEntries(entries); synchronizeLocalMetaData(); - synchronizeLocalDatabase(); // Pull changes for the case that there where some + pullWithLastEntry(); + dbmsProcessor.removeEntries(event.getBibEntries()); + synchronizeLocalDatabase(); } } @@ -313,11 +322,32 @@ public void pullChanges() { if (!checkCurrentConnection()) { return; } - + // First synchronize entry, then synchronize database + pullWithLastEntry(); synchronizeLocalDatabase(); synchronizeLocalMetaData(); } + // Synchronizes local BibEntries only if last entry changes still remain + public void pullLastEntryChanges() { + if (!lastEntryChanged.isEmpty()) { + if (!checkCurrentConnection()) { + return; + } + synchronizeLocalMetaData(); + pullWithLastEntry(); + synchronizeLocalDatabase(); // Pull changes for the case that there were some + } + } + + // Synchronizes local BibEntries and pulls remaining last entry changes + private void pullWithLastEntry() { + if (!lastEntryChanged.isEmpty() && isPresentLocalBibEntry(lastEntryChanged.get())) { + synchronizeSharedEntry(lastEntryChanged.get()); + } + lastEntryChanged = Optional.empty(); + } + /** * Checks whether the current SQL connection is valid. In case that the connection is not valid a new {@link * ConnectionLostEvent} is going to be sent. @@ -360,6 +390,8 @@ public void openSharedDatabase(DatabaseConnection connection) throws DatabaseNot @Override public void closeSharedDatabase() { + // Submit remaining entry changes + pullLastEntryChanges(); try { dbmsProcessor.stopNotificationListener(); currentConnection.close(); diff --git a/src/main/java/org/jabref/logic/util/CoarseChangeFilter.java b/src/main/java/org/jabref/logic/util/CoarseChangeFilter.java index 9c7a7236a3a..ead72efe4d5 100644 --- a/src/main/java/org/jabref/logic/util/CoarseChangeFilter.java +++ b/src/main/java/org/jabref/logic/util/CoarseChangeFilter.java @@ -4,6 +4,7 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.database.event.BibDatabaseContextChangedEvent; +import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.event.FieldChangedEvent; import org.jabref.model.entry.field.Field; @@ -17,54 +18,44 @@ public class CoarseChangeFilter { private final BibDatabaseContext context; private final EventBus eventBus = new EventBus(); - private final DelayTaskThrottler delayPost; private Optional lastFieldChanged; + private Optional lastEntryChanged; private int totalDelta; public CoarseChangeFilter(BibDatabaseContext bibDatabaseContext) { // Listen for change events - bibDatabaseContext.getDatabase().registerListener(this); - bibDatabaseContext.getMetaData().registerListener(this); this.context = bibDatabaseContext; - // Delay event post by 5 seconds - this.delayPost = new DelayTaskThrottler(5000); + context.getDatabase().registerListener(this); + context.getMetaData().registerListener(this); this.lastFieldChanged = Optional.empty(); - this.totalDelta = 0; + this.lastEntryChanged = Optional.empty(); } @Subscribe public synchronized void listen(BibDatabaseContextChangedEvent event) { - Runnable eventPost = () -> { - // Reset total change delta - totalDelta = 0; - // Post event - eventBus.post(event); - }; - if (!(event instanceof FieldChangedEvent)) { - eventPost.run(); - } else { + if (event instanceof FieldChangedEvent) { // Only relay event if the field changes are more than one character or a new field is edited FieldChangedEvent fieldChange = (FieldChangedEvent) event; - // Sum up change delta - totalDelta += fieldChange.getDelta(); - - // If editing is started - boolean isNewEdit = lastFieldChanged.isEmpty(); - // If other field is edited - boolean isEditOnOtherField = !isNewEdit && !lastFieldChanged.get().equals(fieldChange.getField()); - // Only deltas of 1 registered by fieldChange, major change means editing much content - boolean isMajorChange = totalDelta >= 100; - - if ((isEditOnOtherField && !isNewEdit) || isMajorChange) { - // Submit old changes immediately - eventPost.run(); - } else { - delayPost.schedule(eventPost); - } - // Set new last field + + // If editing has started + boolean isNewEdit = lastFieldChanged.isEmpty() || lastEntryChanged.isEmpty(); + + boolean isChangedField = lastFieldChanged.filter(f -> !f.equals(fieldChange.getField())).isPresent(); + boolean isChangedEntry = lastEntryChanged.filter(e -> !e.equals(fieldChange.getBibEntry())).isPresent(); + boolean isEditChanged = !isNewEdit && (isChangedField || isChangedEntry); + // Only deltas of 1 when typing in manually, major change means pasting something (more than one character) + boolean isMajorChange = fieldChange.getDelta() > 1; + + fieldChange.setFilteredOut(!(isEditChanged || isMajorChange)); + // Post each FieldChangedEvent - even the ones being marked as "filtered" + eventBus.post(fieldChange); + lastFieldChanged = Optional.of(fieldChange.getField()); + lastEntryChanged = Optional.of(fieldChange.getBibEntry()); + } else { + eventBus.post(event); } } diff --git a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java index 939d9e5250b..6ce6c35094a 100644 --- a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java +++ b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java @@ -25,7 +25,8 @@ public class DelayTaskThrottler { private static final Logger LOGGER = LoggerFactory.getLogger(DelayTaskThrottler.class); private final ScheduledThreadPoolExecutor executor; - private final int delay; + + private int delay; private ScheduledFuture scheduledTask; @@ -41,7 +42,7 @@ public DelayTaskThrottler(int delay) { public ScheduledFuture schedule(Runnable command) { if (scheduledTask != null) { - scheduledTask.cancel(false); + cancel(); } try { scheduledTask = executor.schedule(command, delay, TimeUnit.MILLISECONDS); @@ -51,9 +52,20 @@ public ScheduledFuture schedule(Runnable command) { return scheduledTask; } + // Execute scheduled Runnable early + public void execute(Runnable command) { + delay = 0; + schedule(command); + } + + // Cancel scheduled Runnable gracefully + public void cancel() { + scheduledTask.cancel(false); + } + public ScheduledFuture scheduleTask(Callable command) { if (scheduledTask != null) { - scheduledTask.cancel(false); + cancel(); } try { scheduledTask = executor.schedule(command, delay, TimeUnit.MILLISECONDS); diff --git a/src/main/java/org/jabref/model/database/event/BibDatabaseContextChangedEvent.java b/src/main/java/org/jabref/model/database/event/BibDatabaseContextChangedEvent.java index 15acdced130..197097691e3 100644 --- a/src/main/java/org/jabref/model/database/event/BibDatabaseContextChangedEvent.java +++ b/src/main/java/org/jabref/model/database/event/BibDatabaseContextChangedEvent.java @@ -1,5 +1,6 @@ package org.jabref.model.database.event; +import org.jabref.model.entry.event.EntriesEvent; import org.jabref.model.groups.event.GroupUpdatedEvent; import org.jabref.model.metadata.event.MetaDataChangedEvent; @@ -7,5 +8,22 @@ * This Event is automatically fired at the same time as {@link EntriesEvent}, {@link GroupUpdatedEvent} or {@link MetaDataChangedEvent}. */ public class BibDatabaseContextChangedEvent { - // no data + // If the event has been filtered out + private boolean filteredOut; + + public BibDatabaseContextChangedEvent() { + this(false); + } + + public BibDatabaseContextChangedEvent(boolean filteredOut) { + this.filteredOut = filteredOut; + } + + public boolean isFilteredOut() { + return filteredOut; + } + + public void setFilteredOut(boolean filtered) { + this.filteredOut = filtered; + } } diff --git a/src/main/java/org/jabref/model/entry/event/EntriesEvent.java b/src/main/java/org/jabref/model/entry/event/EntriesEvent.java index aa0511b1950..e44aa89ca9f 100644 --- a/src/main/java/org/jabref/model/entry/event/EntriesEvent.java +++ b/src/main/java/org/jabref/model/entry/event/EntriesEvent.java @@ -26,6 +26,7 @@ public EntriesEvent(List bibEntries) { * @param location Location affected by this event */ public EntriesEvent(List bibEntries, EntriesEventSource location) { + super(); this.bibEntries = Objects.requireNonNull(bibEntries); this.location = Objects.requireNonNull(location); } diff --git a/src/main/java/org/jabref/model/entry/event/FieldChangedEvent.java b/src/main/java/org/jabref/model/entry/event/FieldChangedEvent.java index 71cff406894..45d9ae2ec1d 100644 --- a/src/main/java/org/jabref/model/entry/event/FieldChangedEvent.java +++ b/src/main/java/org/jabref/model/entry/event/FieldChangedEvent.java @@ -27,7 +27,7 @@ public FieldChangedEvent(BibEntry bibEntry, Field field, String newValue, String this.field = field; this.newValue = newValue; this.oldValue = oldValue; - delta = computeDelta(oldValue, newValue); + this.delta = computeDelta(oldValue, newValue); } /** @@ -40,7 +40,7 @@ public FieldChangedEvent(BibEntry bibEntry, Field field, String newValue, String this.field = field; this.newValue = newValue; this.oldValue = oldValue; - delta = computeDelta(oldValue, newValue); + this.delta = computeDelta(oldValue, newValue); } /** @@ -51,7 +51,7 @@ public FieldChangedEvent(FieldChange fieldChange, EntriesEventSource location) { this.field = fieldChange.getField(); this.newValue = fieldChange.getNewValue(); this.oldValue = fieldChange.getOldValue(); - delta = computeDelta(oldValue, newValue); + this.delta = computeDelta(oldValue, newValue); } public FieldChangedEvent(FieldChange fieldChange) { diff --git a/src/main/java/org/jabref/model/metadata/event/MetaDataChangedEvent.java b/src/main/java/org/jabref/model/metadata/event/MetaDataChangedEvent.java index db3997cb15e..f910e50a47c 100644 --- a/src/main/java/org/jabref/model/metadata/event/MetaDataChangedEvent.java +++ b/src/main/java/org/jabref/model/metadata/event/MetaDataChangedEvent.java @@ -14,6 +14,7 @@ public class MetaDataChangedEvent extends BibDatabaseContextChangedEvent { * @param metaData Affected instance */ public MetaDataChangedEvent(MetaData metaData) { + super(); this.metaData = metaData; }