Add partial reindexing to composite views #4151

imsdu · 2023-08-09T15:09:16Z

Related to #3948

This first part prepares the model for partial reindexing when updating a composite view:

The indexingRev represents the last revision where a change impacted the indexing
Whenever a source is updated, the indexingRev is updated for both sources and projections and it will trigger a full reindexing
If a projection is updated, only this projection will be restarted

The changes in the indexing process will come in a second PR

imsdu · 2023-08-10T09:55:49Z

...rc/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/model/CompositeView.scala

@@ -55,8 +57,8 @@ import scala.concurrent.duration.FiniteDuration
 final case class CompositeView(
    id: Iri,
    project: ProjectRef,
-    sources: NonEmptySet[CompositeViewSource],
-    projections: NonEmptySet[CompositeViewProjection],
+    sources: NonEmptyMap[Iri, CompositeViewSource],


NonEmptySet were kind of a pain here and as we mostly want to access sources and projections individually, a NonEmptyMap makes more sense

imsdu · 2023-08-10T09:56:47Z

...n/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/model/CompositeViewFields.scala

@@ -28,8 +27,8 @@ import scala.concurrent.duration.FiniteDuration
 final case class CompositeViewFields(
    name: Option[String],
    description: Option[String],
-    sources: NonEmptySet[CompositeViewSourceFields],
-    projections: NonEmptySet[CompositeViewProjectionFields],
+    sources: NonEmptyList[CompositeViewSourceFields],


Same, here, we check for duplication before creating the composite view anyway

Is this an advantage? Set is telling us that ordering doesn't matter and the duplicates don't have an effect, which seems good?

Yes because NonEmptyList is a lot easier to use.
NonEmptySet is based on a sorted set, requires an order and did not bring value here
as there is also a check for duplicates on the id of sources/projections when evaluating as sources and projections must all have different ids.

...s/search/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/search/SearchViewFactory.scala

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/circe/package.scala

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/circe/Decoders.scala

delta/sourcing-psql/src/main/scala/ch/epfl/bluebrain/nexus/delta/sourcing/Transactors.scala

...s/search/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/search/SearchViewFactory.scala

olivergrabinski · 2023-08-11T09:16:07Z

...in/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/model/CompositeViewState.scala

@@ -35,6 +35,8 @@ import scala.annotation.nowarn
  *   the collection of tags
  * @param rev
  *   the current revision of the view
+  * @param indexingRev


There is no indexingRev in this class

ℹ️ This is still there

olivergrabinski · 2023-08-11T09:33:59Z

...rc/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/model/CompositeView.scala

+    */
+  def sparqlProjections: Set[SparqlProjection] = projectionsOf[SparqlProjection]
+
+  private def projectionsOf[X: ClassTag] =


It won't looks super nice but it is possible to avoid reflection for sparqlProjections and elasticSearchProjections. Probably doesn't really matter though

olivergrabinski · 2023-08-11T09:37:02Z

...ala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/model/CompositeViewProjection.scala

@@ -37,6 +35,8 @@ sealed trait CompositeViewProjection extends Product with Serializable {
    */
  def uuid: UUID

+  def indexingRev: Int


Utility of the docstring might be questionable but it is missing

olivergrabinski · 2023-08-11T09:57:31Z

...ews/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/CompositeViews.scala

@@ -534,14 +533,9 @@ object CompositeViews {
      case Some(s) if s.deprecated   =>
        IO.raiseError(ViewIsDeprecated(c.id))
      case Some(s)                   =>
+        val newRev = s.rev + 1


When newRev is used here or in further methods I have to say I keep losing track of what kind of revision we are dealing with

Yep, I try to address that

shinyhappydan · 2023-08-11T10:50:46Z

...rc/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/model/CompositeView.scala

+    projections.foldLeft(Set.empty[X]) {
+      case (acc, projection: X) => acc + projection
+      case (acc, _)             => acc
+    }


You could just use .collect here

projections is a Map, not a set

shinyhappydan · 2023-08-11T11:05:22Z

...t/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/CompositeViewFactorySuite.scala

+    includeMetadata
+  )
+
+  test("Create the matching source from the project source field with a defined id") {


What is a matching source?

More broadly I don't really understand what these tests are actually asserting. What is the API doing?

That the fields are correctly transformed into a value

shinyhappydan · 2023-08-11T11:12:36Z

...epfl/bluebrain/nexus/delta/plugins/compositeviews/migration/MigrateCompositeViewsSuite.scala

+  test("Insert states and events and run migration") {
+    for {
+      _ <- xas.initPartitions(proj)
+      // Events to migrate
+      _ <- loadEvent("migration/event-created.json")
+      _ <- loadEvent("migration/event-updated.json")
+      // Events already migrated
+      _ <- loadEvent("composite-views/database/named-view-created.json")
+      _ <- loadEvent("composite-views/database/named-view-updated.json")
+      // States to migrate
+      _ <- loadState(Tag.latest, "migration/state.json")
+      _ <- loadState(UserTag.unsafe("v1.0"), "migration/state.json")
+      _ <- loadState(Tag.latest, "composite-views/database/view-state.json")
+      _ <- loadState(UserTag.unsafe("v1.0"), "composite-views/database/view-state.json")
+      _ <- eventsToMigrate(xas).map(_.assertSize(2))
+      _ <- statesToMigrate(xas).map(_.assertSize(2))
+      _ <- new MigrateCompositeViews(xas).run.assert((2, 2))
+      // Making sure all has been updated
+      _ <- eventsToMigrate(xas).map(_.assertSize(0))
+      _ <- statesToMigrate(xas).map(_.assertSize(0))
+    } yield ()
+  }


This shouldn't be a test, it's just setup

It is not only setup, it also run the migration.

The fact that it's unclear isn't a good sign. If this test runs a migration, and the other tests are checking that the migration ran correctly, then I would say that either this test is actually setup, or there should only be one test

...c/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/CompositeViewFactory.scala

olivergrabinski · 2023-08-14T08:52:32Z

...in/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/model/CompositeViewState.scala

@@ -35,6 +35,8 @@ import scala.annotation.nowarn
  *   the collection of tags
  * @param rev
  *   the current revision of the view
+  * @param indexingRev


ℹ️ This is still there

Add partial reindexing to composite views

3c9570e

imsdu commented Aug 10, 2023

View reviewed changes

olivergrabinski reviewed Aug 10, 2023

View reviewed changes

...s/search/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/search/SearchViewFactory.scala Show resolved Hide resolved

Add migration code for composite views

d2db516

imsdu marked this pull request as ready for review August 11, 2023 07:44

olivergrabinski suggested changes Aug 11, 2023

View reviewed changes

shinyhappydan reviewed Aug 11, 2023

View reviewed changes

Simon Dumas added 4 commits August 11, 2023 16:46

Refactoring and introduce value class IndexingRev

36bc732

Scalafmt

8038a96

Add return type

9e47f41

Fix supervision integration test

6738c6c

olivergrabinski reviewed Aug 14, 2023

View reviewed changes

...c/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/compositeviews/CompositeViewFactory.scala Show resolved Hide resolved

olivergrabinski approved these changes Aug 14, 2023

View reviewed changes

Merge branch 'master' into 3948-composite-views-partial-reindexing

d34a0cf

imsdu merged commit 4f85763 into BlueBrain:master Aug 17, 2023
4 of 5 checks passed

imsdu deleted the 3948-composite-views-partial-reindexing branch August 17, 2023 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add partial reindexing to composite views #4151

Add partial reindexing to composite views #4151

imsdu commented Aug 9, 2023 •

edited

Loading

imsdu Aug 10, 2023

imsdu Aug 10, 2023

shinyhappydan Aug 10, 2023

imsdu Aug 10, 2023

olivergrabinski Aug 11, 2023

olivergrabinski Aug 14, 2023

olivergrabinski Aug 11, 2023

olivergrabinski Aug 11, 2023

olivergrabinski Aug 11, 2023

imsdu Aug 11, 2023

shinyhappydan Aug 11, 2023

imsdu Aug 11, 2023

shinyhappydan Aug 11, 2023

shinyhappydan Aug 11, 2023

imsdu Aug 11, 2023

shinyhappydan Aug 11, 2023

imsdu Aug 11, 2023

shinyhappydan Aug 11, 2023

olivergrabinski Aug 14, 2023

Add partial reindexing to composite views #4151

Add partial reindexing to composite views #4151

Conversation

imsdu commented Aug 9, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsdu commented Aug 9, 2023 •

edited

Loading