[WIP] REP-45 Add option to delete replication data through UI #52

peterhuffer · 2019-03-27T16:01:08Z

What does this PR do?

More tests coming.

Fixes an issue with erroneous data transfer if 2 replications had overlapping filters. The fix was making sure a replication item always has a unique id (changed from metacard id)
Fixes an issue with karaf configuration delete command not cleaning up replication items.
Fixes an issue in the UI where clicking the save configuration button twice quickly would save 2 configurations with the same name
Adds option to delete replicated data when deleting a configuration. Only data replicated from another node to the local node will be deleted.
Add a scheduled deleter that checks for deleted configurations and cleans them up (runs every minute)
Add checks for deleting orphaned replication items (replication items without corresponding configurations or metacards).

Who is reviewing it (please choose AT LEAST two reviewers that need to approve the PR before it can get merged)?

@clockard @paouelle @kcover @mcalcote

How should this be tested? (List steps with links to updated documentation)

Replicate and verify deleting data along with the replication.
Replicate and delete a config (preserver data). Delete a metacard and verify its corresponding replication item entry gets cleaned up.

Any background context you want to provide?

What are the relevant tickets?

Fixes #45

Screenshots (if appropriate)

Checklist:

Documentation Updated
Update / Add Unit Tests
Update / Add Integration Tests

paouelle

Some questions and comments left.

paouelle · 2019-03-28T14:33:52Z

admin/query/src/main/java/org/codice/ditto/replication/admin/query/ReplicationUtils.java

@@ -249,9 +267,18 @@ public ReplicatorConfig getConfigForId(String id) {
    return siteFields;
  }

-  public ListField<ReplicationField> getReplications() {
+  public ListField<ReplicationField> getReplications(boolean filterDeleted) {


✏️ We should move part of this method into the ReplicatorConfigManager as objects(boolean includeDeleted)

paouelle · 2019-03-28T14:35:28Z

admin/query/src/main/java/org/codice/ditto/replication/admin/query/ReplicationUtils.java

+    if (filterDeleted) {
+      configManager
+          .objects()
+          .filter(c -> !c.isDeleted())


Suggested change

.filter(c -> !c.isDeleted())

.filter(((Predicate<ReplicatorConfig>)ReplicationConfig::isDeleted).negate())

paouelle · 2019-03-28T14:38:40Z

admin/query/src/main/java/org/codice/ditto/replication/admin/query/ReplicationUtils.java

    try {
-      configManager.remove(id);
+      ReplicatorConfig config = configManager.get(id);


❓ Should we modify updateReplication() setConfigSuspended() above to check if the config was deleted before deciding to update it? Same thing anywhere else we get a config to update it.
❓ Should we first check here if the object has already been deleted before changing it?

replication-api-impl/pom.xml

replication-api-impl/src/main/java/org/codice/ditto/replication/api/impl/Metacards.java

...-api-impl/src/main/java/org/codice/ditto/replication/api/impl/data/ReplicatorConfigImpl.java

paouelle · 2019-03-28T15:14:30Z

replication-api/src/main/java/org/codice/ditto/replication/api/ReplicationItemManager.java


-  Optional<ReplicationItem> getItem(String id, String source, String destination);
+  Optional<ReplicationItem> getItem(String metacardId, String source, String destination);

  List<ReplicationItem> getItemsForConfig(String configId, int startIndex, int pageSize)


❗️ That start index is concerning. Are we guaranteed that items are always returned in the same order on every queries and that new ones are only added at the end? What about if some are deleted, wouldn't that starting index be impacted and suddenly miss or skip items?

I had similar concerns. I haven't investigated it too much yet though but I will take a closer look.

Pretty sure the index and page size are passed directly on to solr so as long as it is doing it's job we are ok. When we switch out the backend we will need to make sure that is still the case with whatever we use.

paouelle · 2019-03-28T15:14:57Z

replication-api/src/main/java/org/codice/ditto/replication/api/data/ReplicatorConfig.java

+   * @return if {@code true}, delete the associated data replicated by this {@code
+   *     ReplicatorConfig}, otherwise retain the data.
+   */
+  boolean deleteData();


Suggested change

boolean deleteData();

boolean shouldDeleteData();

replication-api/src/main/java/org/codice/ditto/replication/api/data/ReplicatorConfig.java

...-api/src/main/java/org/codice/ditto/replication/api/persistence/ReplicatorConfigManager.java

kcover

I think I might've already asked this question before but why do we have to mark the configs as deleted rather than just deleting them directly?

kcover · 2019-03-28T16:25:53Z

admin/query/src/main/java/org/codice/ditto/replication/admin/query/ReplicationUtils.java

@@ -249,9 +267,18 @@ public ReplicatorConfig getConfigForId(String id) {
    return siteFields;
  }

-  public ListField<ReplicationField> getReplications() {
+  public ListField<ReplicationField> getReplications(boolean filterDeleted) {


✏️ at first glance I wasn't sure if this meant filter out deleted replications or apply a filter so that I just get deleted replications. excludeDeleted or something similar might be more clear.

...ation-api-impl/src/main/java/org/codice/ditto/replication/api/impl/MetacardConfigLoader.java

kcover · 2019-03-28T17:18:37Z

replication-api-impl/src/main/java/org/codice/ditto/replication/api/impl/Metacards.java

+        .map(Result::getMetacard)
+        .map(Metacard::getId)
+        .collect(Collectors.toSet());
+  }


❓ Instead of creating this new method, why not use the getTypeForFilter() method in the calling class and passing in the filter and getId function?

TBH I copy pasted this from the config delete command.

...api-impl/src/main/java/org/codice/ditto/replication/api/impl/ReplicationItemManagerImpl.java

.../main/java/org/codice/ditto/replication/admin/query/sites/persist/DeleteReplicationSite.java

peterhuffer · 2019-03-29T16:01:13Z

I need to close this PR and open it against 0.2.x.

peterhuffer · 2019-03-29T16:13:13Z

@kcover Because if a config replicates millions of resources, it could take quite awhile to delete, and we would not want to block the UI for that.

peterhuffer mentioned this pull request Mar 27, 2019

Various bug fixes #53

Merged

3 tasks

root added 2 commits March 28, 2019 06:17

[maven-release-plugin] prepare release replication-0.2.1

d86a0d0

[maven-release-plugin] prepare for next development iteration

3cd0c8b

paouelle suggested changes Mar 28, 2019

View reviewed changes

kcover reviewed Mar 28, 2019

View reviewed changes

REP-45 Add option to delete replication data through UI

a7000f3

peterhuffer force-pushed the REP-45 branch from 65c3768 to a7000f3 Compare March 29, 2019 16:00

peterhuffer closed this Mar 29, 2019

peterhuffer mentioned this pull request Mar 29, 2019

REP-45 Add option to delete replication data through UI #60

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] REP-45 Add option to delete replication data through UI #52

[WIP] REP-45 Add option to delete replication data through UI #52

peterhuffer commented Mar 27, 2019

paouelle left a comment

paouelle Mar 28, 2019

paouelle Mar 28, 2019

paouelle Mar 28, 2019

paouelle Mar 28, 2019

peterhuffer Mar 28, 2019

clockard Mar 29, 2019

paouelle Mar 28, 2019

kcover left a comment

kcover Mar 28, 2019

kcover Mar 28, 2019

peterhuffer Mar 29, 2019

peterhuffer commented Mar 29, 2019

peterhuffer commented Mar 29, 2019

	.filter(c -> !c.isDeleted())
	.filter(((Predicate<ReplicatorConfig>)ReplicationConfig::isDeleted).negate())

[WIP] REP-45 Add option to delete replication data through UI #52

[WIP] REP-45 Add option to delete replication data through UI #52

Conversation

peterhuffer commented Mar 27, 2019

What does this PR do?

Who is reviewing it (please choose AT LEAST two reviewers that need to approve the PR before it can get merged)?

How should this be tested? (List steps with links to updated documentation)

Any background context you want to provide?

What are the relevant tickets?

Screenshots (if appropriate)

Checklist:

paouelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuffer commented Mar 29, 2019

peterhuffer commented Mar 29, 2019