Feat:split pagination and fragmentation #1294

pj-cegeka · 2024-05-24T10:25:37Z

No description provided.

Yalz

Some general remarks that I don't find the right place for it:

I was thinking that the split would maybe mean a separate module since i see no logic that should be shared between these two functionalities? (this would mean that my table name suggestion needs to be modified of course)
The current PAginationService seems quite complex with the isRunning, stopTask, ... and should in the next iteration best use Spring Batch (this however doesn't stop this PR)

Yalz · 2024-05-24T11:48:20Z

docker-compose/server.config.yml

    hibernate:
      ddl-auto: update


since liquibase will take care of our tables, this should be dropped. (also from the docs)

Yalz · 2024-05-24T11:57:15Z

...vlaanderen/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyExecutor.java

-										 MemberRetriever memberRetriever,
-                                         FragmentSequenceRepository fragmentSequenceRepository) {
+                                         MemberRetriever memberRetriever,
+                                         FragmentSequenceRepository fragmentSequenceRepository, BucketisedMemberSaverImpl bucketisedMemberSaver) {


If you could use the Interface instead of the implementation here..?

Yalz · 2024-05-24T11:59:42Z

.../be/vlaanderen/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyImpl.java

+	public List<BucketisedMember> addMemberToFragment(Fragment fragment, Member member,
+													  Observation parentObservation) {
+		return List.of(new BucketisedMember(member.id(), fragment.getViewName(), fragment.getFragmentIdString(), member.sequenceNr()));


so if i get this right, the FragmentationStrategyImpl is the lowest level of fragmentation which was actually the process that did the pagination.. How come this is then still in place?

The way I see it, was this lowest level impl responsible for saving it to the repository and letting the rest of the application know the fragmentation of the member is completed. However, this is moved to the BucketisedMemberSaverImpl which does this job now and the only job this impl does, is mapping the member to a singleton BucketisedMember list.

However, I am not a fan of these changes, as it seems to break the decorator pattern and a lot of refactoring was required, as the method now needs to return the list rather than handling the saving into the repository and publishing the event by the lowest level impl.
In addition due to these changes, the injected fields are not used anymore

Yalz · 2024-05-24T12:25:00Z

...nformatievlaanderen/ldes/server/fragmentation/postgres/entity/MemberBucketisationEntity.java

+        @Index(columnList = "sequenceNr")
+})
+public class MemberBucketisationEntity {
+    public static final String BUCKETISATION = "bucketisation";


can you prefix this ? fragmentation_bucket

also in general, i'd call in memberBucketEntity since otherwise it sounds too much like the process instead of a table

Yalz · 2024-05-24T12:27:12Z

ldes-server-infra-postgres/postgres-liquibase/pom.xml

+    <properties>
+        <maven.compiler.source>21</maven.compiler.source>
+        <maven.compiler.target>21</maven.compiler.target>
+        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+    </properties>


please exclude these properties :)

Yalz · 2024-05-24T12:29:17Z

...er-infra-postgres/postgres-liquibase/src/main/resources/db/changelog/3_0_1/bucketisation.sql

@@ -0,0 +1,12 @@
+create table public.bucketisation


Would reccomend it to split creation off in the XML approach, looks a bit cleaner :)

Yalz · 2024-05-24T12:30:02Z

ldes-server-infra-postgres/postgres-liquibase/src/main/resources/db/changelog/master.xml

+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-2.0.xsd">
+
+    <include file="/db/changelog/3_0_1/bucketisation.sql" />


I'm missing the provided configs i passed on? 😄

Yalz · 2024-05-24T12:33:17Z

...deren/informatievlaanderen/ldes/server/fragmentisers/pagination/MemberPaginationService.java

+            fragmentSequenceRepository.saveLastProcessedSequence(new FragmentSequence(viewName, sequenceNr + 1));
+            sequenceNr += 1;


if you do sequenceNr++, the increment will not be added before the computation of the FragmentSequence i believe

I believe so too, however, instead of sequenceNr++, ++sequenceNr could be used, which should do the addition beforehand

Yalz · 2024-05-24T12:36:18Z

...nformatievlaanderen/ldes/server/fragmentation/postgres/entity/MemberBucketisationEntity.java

+    @Id
+    private String id;


if not using the String as an identifier, I'd use a Auto increment value as ID and store the bucket name as a separate value. This mainly has impact on peformance when it comes to indexing.

jobulcke · 2024-05-24T13:29:21Z

.../be/vlaanderen/informatievlaanderen/ldes/server/fragmentation/entities/BucketisedMember.java

As this is an entity, a custom equals and hashcode impl should be provided that only checks the ids

jobulcke · 2024-05-24T13:31:19Z

.../be/vlaanderen/informatievlaanderen/ldes/server/fragmentation/BucketisedMemberSaverImpl.java

+import java.util.List;
+
+@Component
+public class BucketisedMemberSaverImpl {


I think it is odd that the class name contains the suffix "Impl" when it does not implement an interface. I would suggest to either extract an interface from this or remove the suffix

jobulcke · 2024-05-24T13:39:47Z

...vlaanderen/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyExecutor.java

@@ -24,23 +25,25 @@ public class FragmentationStrategyExecutor {
 	private final ObservationRegistry observationRegistry;
 	private final MemberRetriever memberRetriever;
 	private final FragmentSequenceRepository fragmentSequenceRepository;
+	private final BucketisedMemberSaverImpl bucketisedMemberSaver;
 	private boolean isExecutorActive = true;

 	public FragmentationStrategyExecutor(ViewName viewName,


A new sonar will be introduced, as this constructor has more than 7 parameters, so could be either suppressed or fixed please?

jobulcke · 2024-05-24T13:48:22Z

.../be/vlaanderen/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyImpl.java

+	public List<BucketisedMember> addMemberToFragment(Fragment fragment, Member member,
+													  Observation parentObservation) {
+		return List.of(new BucketisedMember(member.id(), fragment.getViewName(), fragment.getFragmentIdString(), member.sequenceNr()));


The way I see it, was this lowest level impl responsible for saving it to the repository and letting the rest of the application know the fragmentation of the member is completed. However, this is moved to the BucketisedMemberSaverImpl which does this job now and the only job this impl does, is mapping the member to a singleton BucketisedMember list.

jobulcke · 2024-05-24T13:54:34Z

.../be/vlaanderen/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyImpl.java

+	public List<BucketisedMember> addMemberToFragment(Fragment fragment, Member member,
+													  Observation parentObservation) {
+		return List.of(new BucketisedMember(member.id(), fragment.getViewName(), fragment.getFragmentIdString(), member.sequenceNr()));


However, I am not a fan of these changes, as it seems to break the decorator pattern and a lot of refactoring was required, as the method now needs to return the list rather than handling the saving into the repository and publishing the event by the lowest level impl.
In addition due to these changes, the injected fields are not used anymore

jobulcke · 2024-05-24T14:00:03Z

...ren/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyCreatorImplTest.java


 		FragmentationStrategy fragmentationStrategy = fragmentationStrategyCreator
 				.createFragmentationStrategyForView(viewSpecification);

-		assertEquals(paginationFragmentationStrategy, fragmentationStrategy);
+		assertThat(fragmentationStrategy).isOfAnyClassIn(FragmentationStrategyImpl.class);


As you are only checking for one class type, I would suggest to use isInstanceOf() instead of isOfAnyClassIn()

jobulcke · 2024-05-24T14:04:00Z

...deren/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyDecoratorTest.java

+		fragmentationStrategyDecorator.addMemberToFragment(parentFragment, member,
 				span);


Formatting seems off

jobulcke · 2024-05-24T14:09:27Z

...vlaanderen/informatievlaanderen/ldes/server/fragmentation/FragmentationStrategyImplTest.java

+		assertThat(members.getFirst()).hasFieldOrPropertyWithValue("memberId", MEMBER_ID)
+				.hasFieldOrPropertyWithValue("viewName", VIEW_NAME)
+				.hasFieldOrPropertyWithValue("fragmentId", FRAGMENT_ID.asDecodedFragmentId())
+				.hasFieldOrPropertyWithValue("sequenceNr", SEQ_NR);


A little suggestion: an expectedBucketisedMember could be declared with all these properties and then the assertion could look like this:

assertThat(members.getFirst()) .usingRecursiveComparison() // this makes sure all fields are compared .isEqualTo(expectedBucketisedMember);

jobulcke · 2024-05-24T14:14:42Z

...deren/informatievlaanderen/ldes/server/fragmentisers/pagination/MemberPaginationService.java

+            fragmentSequenceRepository.saveLastProcessedSequence(new FragmentSequence(viewName, sequenceNr + 1));
+            sequenceNr += 1;


I believe so too, however, instead of sequenceNr++, ++sequenceNr could be used, which should do the addition beforehand

sonarcloud · 2024-05-29T15:15:00Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
79.0% Coverage on New Code
1.5% Duplication on New Code

See analysis details on SonarCloud

pj-cegeka added 7 commits May 24, 2024 03:41

temp

ea3ef64

temp

ce876e3

temp

c3715bc

temp

8840a00

Fix: fix tests

c863ab8

Fix: fix delete view

f88c5be

feat/ liquibase migration

31a34ae

pj-cegeka requested review from Yalz, rorlic, jobulcke and ferre-vaes as code owners May 24, 2024 10:25

pj-cegeka added 2 commits May 24, 2024 13:32

feat: add tests

1fce4db

feat: add tests

04382f2

Yalz reviewed May 24, 2024

View reviewed changes

jobulcke reviewed May 24, 2024

View reviewed changes

pj-cegeka added 15 commits May 28, 2024 09:34

feat: create pagination module

cb310e4

feat: fix liquibase

1f79a30

feat: fix test

53d3507

feat: fix pom

40cf241

feat: fix pom

f997d25

feat: fix test

d7be011

feat: fix test

35ca07a

feat: fix test

b1c34f3

feat: fix test

bfeaa62

feat: fix test

b139c18

feat: fix test

49d22e5

feat: fix test

4f8c682

feat: fix test

bab8643

feat: fix test

db745d5

feat: fix test

a93e0a6

pj-cegeka added 7 commits May 29, 2024 11:47

feat: fix test

9390789

feat: fix test

72b56c9

feat: fix test

d465b99

feat: fix test

4adcd81

feat: fix test

a72ccc8

feat: fix test

dc1e641

fix: issues

7561e0c

Yalz approved these changes May 30, 2024

View reviewed changes

pj-cegeka merged commit 6a80179 into develop May 30, 2024
3 checks passed

pj-cegeka deleted the feat/split-migration-liquibase branch May 30, 2024 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat:split pagination and fragmentation #1294

Feat:split pagination and fragmentation #1294

pj-cegeka commented May 24, 2024

Yalz left a comment

Yalz May 24, 2024

Yalz May 24, 2024

Yalz May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

Yalz May 24, 2024

Yalz May 24, 2024

Yalz May 24, 2024

Yalz May 24, 2024

Yalz May 24, 2024

Yalz May 24, 2024

jobulcke May 24, 2024

Yalz May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

jobulcke May 24, 2024

sonarcloud bot commented May 29, 2024

		fragmentSequenceRepository.saveLastProcessedSequence(new FragmentSequence(viewName, sequenceNr + 1));
		sequenceNr += 1;

		fragmentationStrategyDecorator.addMemberToFragment(parentFragment, member,
		span);

Feat:split pagination and fragmentation #1294

Feat:split pagination and fragmentation #1294

Conversation

pj-cegeka commented May 24, 2024

Yalz 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

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

sonarcloud bot commented May 29, 2024

Quality Gate passed