Feature/230 get partitioning checkpoints #265

salamonpavel · 2024-09-05T14:57:31Z

Introduces GET partitioning checkpoints endpoint with pagination
Closes #230

Release notes:

Introduces GET partitioning checkpoints endpoint with pagination

salamonpavel · 2024-09-05T14:57:51Z

Release notes:

Introduces GET partitioning checkpoints endpoint with pagination

…kpoints # Conflicts: # server/src/main/scala/za/co/absa/atum/server/api/repository/PartitioningRepositoryImpl.scala

github-actions · 2024-09-06T11:08:47Z

JaCoCo model module code coverage report - scala 2.13.11

Overall Project	63.64%	🍏

There is no coverage information present for the Files changed

github-actions · 2024-09-06T11:08:48Z

JaCoCo agent module code coverage report - scala 2.13.11

Overall Project	84.63%	🍏

There is no coverage information present for the Files changed

github-actions · 2024-09-06T11:08:49Z

JaCoCo reader module code coverage report - scala 2.13.11

Overall Project	100%	🍏

There is no coverage information present for the Files changed

github-actions · 2024-09-06T11:08:51Z

JaCoCo server module code coverage report - scala 2.13.11

Overall Project	75.41% `-8.05%`	🍏
Files changed	71.79%	❌

File	Coverage
CheckpointRepositoryImpl.scala	100%	🍏
PartitioningRepositoryImpl.scala	100%	🍏
CheckpointServiceImpl.scala	100%	🍏
PartitioningServiceImpl.scala	100%	🍏
BaseController.scala	100%	🍏
CheckpointControllerImpl.scala	100% `-2.9%`	🍏
PartitioningControllerImpl.scala	87.36%	🍏
CheckpointItemFromDB.scala	68.5% `-19.69%`	❌
GetPartitioningCheckpoints.scala	58.1% `-81.33%`	❌

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql

lsulak

LGTM, I like it, I just found a few tiny improvement ideas

...in/scala/za/co/absa/atum/server/api/database/runs/functions/GetPartitioningCheckpoints.scala

# Conflicts: # server/src/main/scala/za/co/absa/atum/server/api/controller/PartitioningControllerImpl.scala # server/src/main/scala/za/co/absa/atum/server/api/repository/PartitioningRepositoryImpl.scala

benedeki · 2024-09-17T12:28:46Z

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql

        status := 41;
        status_text := 'Partitioning not found';
        RETURN NEXT;
        RETURN;
    END IF;

+    IF i_checkpoints_limit IS NOT NULL THEN
+        SELECT count(*) > i_checkpoints_limit


Alternate solution, no need to change the current one, just for inspiration:

PERFORM 1 FROM runs.checkpoints C WHERE C.fk_partitioning = i_partitioning_id AND (i_checkpoint_name IS NULL OR C.checkpoint_name = i_checkpoint_name) LIMIT 1 OFFSET coalesce(i_offset) + i_checkpoints_limit; _has_more := found;

benedeki · 2024-09-17T12:39:51Z

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql

+        FROM runs.checkpoints C
+        WHERE C.fk_partitioning = i_partitioning_id
+          AND (i_checkpoint_name IS NULL OR C.checkpoint_name = i_checkpoint_name)
+        LIMIT i_checkpoints_limit + 1 OFFSET i_offset


Complicated
First I thought this was a mistake to have ORDER BY omitted from the query.
Then I thought: wait we don't care about the order of we just want to see if there are more records.
Then I thought: but having the ORDER BY will cause to have the same data pages cached in memory for the query bellow.
And then I thought: but the ordering might be more expensive then the gain the cache gives.
So I don't know 🤣

Suggested change

LIMIT i_checkpoints_limit + 1 OFFSET i_offset

ORDER BY C.process_start_time DESC, C.id_checkpoint

LIMIT i_checkpoints_limit + 1 OFFSET i_offset

let's keep the code as is then for now :)

benedeki · 2024-09-17T12:41:54Z

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql

-            runs.measurements M ON C.id_checkpoint = M.fk_checkpoint
-        JOIN
+            limited_checkpoints LC
+                JOIN


Personal quirk, no need to change here:
I do prefer INNER JOIN to JOIN to make it obvious. So if in future you would consider using that 👼

ok, will add the 'INNER' and do so going forward always

benedeki · 2024-09-17T12:46:10Z

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql

-            C.process_start_time,
-            C.id_checkpoint
-        LIMIT nullif(i_limit, 0);
+            LC.process_start_time, LC.id_checkpoint;


Major:
We want the measurements of one checkpoint go after each other followed by the next checkpoint etc.
While the current way how the measurements come in (all at once) ensure this gives the correct order, this is not guaranteed by the data itself, just by actual processes right now.

Suggested change

LC.process_start_time, LC.id_checkpoint;

LC.id_checkpoint, LC.process_start_time;

ok switched the order

benedeki · 2024-09-17T12:47:36Z

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql


+    IF NOT FOUND THEN


Is this a problem?
I think it's valid situation, if no checkpoint for an existing partitioning exist so far.

Actually, so far in queries returning detail records of a master one, of there were no details one, we ust returned the empty set. Would prefer we stay consistent, and if for some reason this is considered better, changer there too.
Tag @TebaleloS @lsulak

I think you're right. How about status 12 with text 'OK with no checkpoints found'?

I think the OK solution might cause even more problems, as the data returned are NULL. So fa-db would need to be able to handle that and recognize that the particular record is OK but return empty sequence.
In the other cases we just returned no records at all.

benedeki · 2024-09-17T12:55:04Z

server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala

-      .in(GetPartitioningCheckpoints)
-      .in(jsonBody[CheckpointQueryDTO])
+      .in(V2Paths.Partitionings / path[Long]("partitioningId") / V2Paths.Checkpoints)
+      .in(query[Option[Int]]("limit").default(Some(10)).validateOption(Validator.inRange(1, 1000)))


Minor:
Don't we want to put the (upper) limit to constants? Or we might consider different limit for each paginated endpoint?

I agree we could maybe have some base endpoint for all/most paginated endpoints so we don't have to repeat this for every single one of them. Didn't want to optimize it yet given it's the first paginated endpoint ...

Can do later sure. As hinted, this is minor thing, aboslutely not blocking.

benedeki · 2024-09-17T18:22:29Z

Any chance to increase the coverage %, @salamonpavel ?

benedeki · 2024-09-18T10:51:37Z

Any chance to increase the coverage %, @salamonpavel ?

Coincidentally @miroslavpojer today mentioned that jacoco-report had release version 1.7.1 with number of bugifxes. Can you please increase the version in the script to test and see how the new version works.

salamonpavel · 2024-09-18T20:29:29Z

Any chance to increase the coverage %, @salamonpavel ?

Coincidentally @miroslavpojer today mentioned that jacoco-report had release version 1.7.1 with number of bugifxes. Can you please increase the version in the script to test and see how the new version works.

Those coverage numbers are incorrect.

miroslavpojer · 2024-09-19T06:33:52Z

Any chance to increase the coverage %, @salamonpavel ?

Coincidentally @miroslavpojer today mentioned that jacoco-report had release version 1.7.1 with number of bugifxes. Can you please increase the version in the script to test and see how the new version works.

Those coverage numbers are incorrect.

The new jacoco-report version explains the meaning of the negative percentage numbers.

The "delta" (the negative value next to the coverage) represents the percentage of newly added or modified lines of code that are not covered by unit tests. It calculates the difference in test coverage based solely on the changes made in the current commit or pull request. For example, if 10 lines of code are modified or added, and 8 of those lines are covered by unit tests, the "delta" would be -20%, indicating 20% of the newly changed code is untested. However, the "delta" has limitations. It can never be positive, meaning if you add more unit tests to cover existing, unmodified code, this additional coverage is not reflected in the delta. The metric only considers lines directly changed in the current set of modifications, as there is no mechanism to track improvements in coverage for pre-existing code that hasn't been altered.

Do the numbers make more sense with this explanation?
I still need the action control to switch this feature off.

salamonpavel · 2024-09-19T08:06:41Z

Any chance to increase the coverage %, @salamonpavel ?

Coincidentally @miroslavpojer today mentioned that jacoco-report had release version 1.7.1 with number of bugifxes. Can you please increase the version in the script to test and see how the new version works.

Those coverage numbers are incorrect.

The new jacoco-report version explains the meaning of the negative percentage numbers.

The "delta" (the negative value next to the coverage) represents the percentage of newly added or modified lines of code that are not covered by unit tests. It calculates the difference in test coverage based solely on the changes made in the current commit or pull request. For example, if 10 lines of code are modified or added, and 8 of those lines are covered by unit tests, the "delta" would be -20%, indicating 20% of the newly changed code is untested. However, the "delta" has limitations. It can never be positive, meaning if you add more unit tests to cover existing, unmodified code, this additional coverage is not reflected in the delta. The metric only considers lines directly changed in the current set of modifications, as there is no mechanism to track improvements in coverage for pre-existing code that hasn't been altered.

Do the numbers make more sense with this explanation? I still need the action control to switch this feature off.

Every single line in GetPartitioningCheckpoints.scala is invoked when testing the code yet it reports coverage of 58.1 %.

miroslavpojer · 2024-09-19T10:59:36Z

Any chance to increase the coverage %, @salamonpavel ?

Coincidentally @miroslavpojer today mentioned that jacoco-report had release version 1.7.1 with number of bugifxes. Can you please increase the version in the script to test and see how the new version works.

Those coverage numbers are incorrect.

The new jacoco-report version explains the meaning of the negative percentage numbers.
The "delta" (the negative value next to the coverage) represents the percentage of newly added or modified lines of code that are not covered by unit tests. It calculates the difference in test coverage based solely on the changes made in the current commit or pull request. For example, if 10 lines of code are modified or added, and 8 of those lines are covered by unit tests, the "delta" would be -20%, indicating 20% of the newly changed code is untested. However, the "delta" has limitations. It can never be positive, meaning if you add more unit tests to cover existing, unmodified code, this additional coverage is not reflected in the delta. The metric only considers lines directly changed in the current set of modifications, as there is no mechanism to track improvements in coverage for pre-existing code that hasn't been altered.
Do the numbers make more sense with this explanation? I still need the action control to switch this feature off.

Every single line in GetPartitioningCheckpoints.scala is invoked when testing the code yet it reports coverage of 58.1 %.

The reason for 58,1% is in jacoco.xml
I have retested it on my local, and there is 100% coverage after our scala filtering. It looks like scala filtering was not fully applied for XML file. This is the next big task for the next QA Squad - move the Jacoco solution into the origin repository.

benedeki · 2024-09-20T10:17:06Z

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql

+    IF NOT FOUND THEN
+        status := 12;
+        status_text := 'OK with no checkpoints found';
+        RETURN NEXT;
+    END IF;


Suggested change

IF NOT FOUND THEN

status := 12;

status_text := 'OK with no checkpoints found';

RETURN NEXT;

END IF;

salamonpavel added 2 commits September 5, 2024 16:55

tmp commit

6b2df24

tmp commit

4664180

salamonpavel added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Sep 5, 2024

salamonpavel self-assigned this Sep 5, 2024

salamonpavel added 3 commits September 6, 2024 10:06

sql change

099ce5a

tests

2bd1548

tests

e4cf419

salamonpavel marked this pull request as ready for review September 6, 2024 10:30

salamonpavel requested review from benedeki, lsulak, TebaleloS, Zejnilovic and dk1844 as code owners September 6, 2024 10:30

salamonpavel added 3 commits September 6, 2024 12:32

Merge branch 'refs/heads/master' into feature/230-getPartitioningChec…

3e69c7f

…kpoints # Conflicts: # server/src/main/scala/za/co/absa/atum/server/api/repository/PartitioningRepositoryImpl.scala

conflicts

d59d7da

fex

eb01623

salamonpavel removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Sep 6, 2024

fex

1b28b7c

Merge branch 'master' into feature/230-getPartitioningCheckpoints

4a5e925

lsulak reviewed Sep 11, 2024

View reviewed changes

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql Outdated Show resolved Hide resolved

lsulak reviewed Sep 11, 2024

View reviewed changes

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql Show resolved Hide resolved

lsulak reviewed Sep 11, 2024

View reviewed changes

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql Outdated Show resolved Hide resolved

lsulak reviewed Sep 11, 2024

View reviewed changes

database/src/main/postgres/runs/V1.8.3__get_partitioning_checkpoints.sql Show resolved Hide resolved

lsulak reviewed Sep 11, 2024

View reviewed changes

...in/scala/za/co/absa/atum/server/api/database/runs/functions/GetPartitioningCheckpoints.scala Show resolved Hide resolved

salamonpavel added 3 commits September 16, 2024 14:26

comments addressed

0f15e1c

comments addressed

2bf339e

fix

dd3b7e2

lsulak previously approved these changes Sep 17, 2024

View reviewed changes

salamonpavel added 2 commits September 17, 2024 13:21

Merge branch 'master' into feature/230-getPartitioningCheckpoints

71c1c33

# Conflicts: # server/src/main/scala/za/co/absa/atum/server/api/controller/PartitioningControllerImpl.scala # server/src/main/scala/za/co/absa/atum/server/api/repository/PartitioningRepositoryImpl.scala

conflicts with master resolved

611ea75

salamonpavel dismissed lsulak’s stale review via 611ea75 September 17, 2024 11:24

lsulak previously approved these changes Sep 17, 2024

View reviewed changes

benedeki reviewed Sep 17, 2024

View reviewed changes

comments addressed

7811f37

salamonpavel dismissed lsulak’s stale review via 7811f37 September 18, 2024 12:02

salamonpavel added 5 commits September 18, 2024 14:14

tests fixed

c33a136

tests deterministic

4e0f6ba

jacoco report action 1.7.1

e187745

test

92a2099

remove serde in paginated result

1e14e10

Merge branch 'master' into feature/230-getPartitioningCheckpoints

4b2a9db

benedeki reviewed Sep 20, 2024

View reviewed changes

no status when no results

8811b4e

benedeki mentioned this pull request Sep 25, 2024

#262: Full Flyway integration #276

Merged

benedeki approved these changes Sep 26, 2024

View reviewed changes

salamonpavel merged commit 43ec036 into master Sep 26, 2024
9 of 10 checks passed

salamonpavel deleted the feature/230-getPartitioningCheckpoints branch September 26, 2024 14:30

benedeki mentioned this pull request Sep 27, 2024

get flow checkpoints endpoint v2: 233 #264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/230 get partitioning checkpoints #265

Feature/230 get partitioning checkpoints #265

salamonpavel commented Sep 5, 2024 •

edited by miroslavpojer

Loading

salamonpavel commented Sep 5, 2024

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

lsulak left a comment

benedeki Sep 17, 2024 •

edited

Loading

benedeki Sep 17, 2024

salamonpavel Sep 18, 2024

benedeki Sep 17, 2024

salamonpavel Sep 18, 2024

benedeki Sep 17, 2024 •

edited

Loading

salamonpavel Sep 18, 2024

benedeki Sep 17, 2024

benedeki Sep 18, 2024

salamonpavel Sep 18, 2024

benedeki Sep 19, 2024

benedeki Sep 17, 2024 •

edited

Loading

salamonpavel Sep 18, 2024

benedeki Sep 19, 2024

benedeki commented Sep 17, 2024

benedeki commented Sep 18, 2024

salamonpavel commented Sep 18, 2024

miroslavpojer commented Sep 19, 2024 •

edited

Loading

salamonpavel commented Sep 19, 2024

miroslavpojer commented Sep 19, 2024 •

edited

Loading

benedeki Sep 20, 2024

	LIMIT i_checkpoints_limit + 1 OFFSET i_offset
	ORDER BY C.process_start_time DESC, C.id_checkpoint
	LIMIT i_checkpoints_limit + 1 OFFSET i_offset

	LC.process_start_time, LC.id_checkpoint;
	LC.id_checkpoint, LC.process_start_time;

Feature/230 get partitioning checkpoints #265

Feature/230 get partitioning checkpoints #265

Conversation

salamonpavel commented Sep 5, 2024 • edited by miroslavpojer Loading

salamonpavel commented Sep 5, 2024

github-actions bot commented Sep 6, 2024 • edited Loading

JaCoCo model module code coverage report - scala 2.13.11

github-actions bot commented Sep 6, 2024 • edited Loading

JaCoCo agent module code coverage report - scala 2.13.11

github-actions bot commented Sep 6, 2024 • edited Loading

JaCoCo reader module code coverage report - scala 2.13.11

github-actions bot commented Sep 6, 2024 • edited Loading

JaCoCo server module code coverage report - scala 2.13.11

lsulak left a comment

Choose a reason for hiding this comment

benedeki Sep 17, 2024 • 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

benedeki Sep 17, 2024 • 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

benedeki Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedeki commented Sep 17, 2024

benedeki commented Sep 18, 2024

salamonpavel commented Sep 18, 2024

miroslavpojer commented Sep 19, 2024 • edited Loading

salamonpavel commented Sep 19, 2024

miroslavpojer commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

salamonpavel commented Sep 5, 2024 •

edited by miroslavpojer

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

benedeki Sep 17, 2024 •

edited

Loading

benedeki Sep 17, 2024 •

edited

Loading

benedeki Sep 17, 2024 •

edited

Loading

miroslavpojer commented Sep 19, 2024 •

edited

Loading

miroslavpojer commented Sep 19, 2024 •

edited

Loading