#303: Get flow checkpoints refactoring #304

salamonpavel · 2024-11-21T10:39:14Z

Refactoring of get flow checkpoints endpoint. It newly returns also partitioning data and is sorted in descending order by checkpoint start time.
Closes #303

Release notes:

Get flow checkpoints endpoint refactored to reverse the order of data returned and to include partitioning data.

salamonpavel · 2024-11-21T10:48:36Z

Release notes:

Get flow checkpoints endpoint refactored to reverse the order of data returned and to include partitioning data.

github-actions · 2024-11-21T10:53:22Z

JaCoCo model module code coverage report - scala 2.13.11

Overall Project	59.69%	🍏

There is no coverage information present for the Files changed

github-actions · 2024-11-21T10:53:23Z

JaCoCo agent module code coverage report - scala 2.13.11

Overall Project	78.49%	🍏

There is no coverage information present for the Files changed

github-actions · 2024-11-21T10:53:24Z

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-11-21T10:53:26Z

JaCoCo server module code coverage report - scala 2.13.11

Overall Project	68.39% `-12.8%`	🍏
Files changed	60.68%	❌

File	Coverage
FlowRepositoryImpl.scala	100%	🍏
FlowServiceImpl.scala	100%	🍏
FlowControllerImpl.scala	100%	🍏
CheckpointItemWithPartitioningFromDB.scala	69.04%	❌
GetFlowCheckpoints.scala	53.28% `-58.32%`	❌

database/README.md

database/src/main/postgres/flows/V0.3.0.1__get_flow_checkpoints.sql

model/src/main/scala/za/co/absa/atum/model/dto/CheckpointWithPartitioningDTO.scala

server/src/main/scala/za/co/absa/atum/server/model/CheckpointItemWithPartitioningFromDB.scala

benedeki · 2024-11-22T12:31:59Z

server/src/main/scala/za/co/absa/atum/server/model/CheckpointItemWithPartitioningFromDB.scala

+
+object CheckpointItemWithPartitioningFromDB {
+
+  private def fromItemsToCheckpointWithPartitioningDTO(


Name suggestion:
Wouldn't be def groupCheckpoint be easier to understand?

I don't know. Naming is difficult...

That's true 😄

server/src/main/scala/za/co/absa/atum/server/model/CheckpointItemWithPartitioningFromDB.scala

Co-authored-by: David Benedeki <[email protected]>

…-refactoring' into feature/303-get-flow-checkpoints-refactoring

…temWithPartitioningFromDB.scala Co-authored-by: David Benedeki <[email protected]>

benedeki

code reviewed
pulled
built
run

ABLL526

Looks good to me.

lsulak · 2024-11-25T15:34:16Z

server/src/main/scala/za/co/absa/atum/server/model/CheckpointItemWithPartitioningFromDB.scala

+  checkpointStartTime: ZonedDateTime,
+  checkpointEndTime: Option[ZonedDateTime],
+  idPartitioning: Long,
+  partitioning: Json,


possibly add code comment similar to the one above (the one on measurementValue )

Ok, added ...

lsulak · 2024-11-25T15:42:48Z

database/src/main/postgres/flows/V0.3.0.1__get_flow_checkpoints.sql

+            runs.measure_definitions MD ON M.fk_measure_definition = MD.id_measure_definition
+                INNER JOIN
+            runs.partitionings P ON LC.fk_partitioning = P.id_partitioning
+        ORDER BY LC.process_start_time DESC;


Suggested change

ORDER BY LC.process_start_time DESC;

ORDER BY

CASE WHEN i_latest_first THEN LC.process_start_time DESC

ELSE LC.process_start_time ASC

END;

potentially this should work (pseudo-code, I haven't run it), also this solution would need to introduce 1 more input parameter i_latest_first (can be an optional parameter that is true by default) to the function and make change above on line 135

also, don't we want to make checkpoints closer together by changing this ORDER BY to also contain checkpoint ID in it?

I think it doesn't really matter since we need to process the data on the server anyway.

Ok, added conditional ordering ...

lsulak

I fundamentally don't see any problem with this PR apart from the minor comments currently on it

lsulak

Good job!

benedeki · 2024-11-26T11:20:36Z

.gitignore

@@ -91,3 +91,4 @@ utils/resources/*.conf
 /server/certs/
 /server/selfsigned.crt
 /server/selfsigned.p12
+/.bloop/


I don't know which plugin/tool does that but there was .bloop folder generated which I believe shouldn't be part of VCS.

benedeki · 2024-11-26T13:52:26Z

database/src/main/postgres/flows/V0.3.0.1__get_flow_checkpoints.sql

+                    WHEN i_latest_first THEN C.process_start_time
+                    END DESC,
+                CASE
+                    WHEN NOT i_latest_first THEN C.process_start_time
+                    END ASC


This is tricky on NULL.
Would use an ELSE statement. And probably flip them. So the default is still DESC

Or just use a local variable at the beginning
_latest_first := coalesce(i_latest_first, TRUE);

benedeki · 2024-11-26T13:56:43Z

database/src/main/postgres/flows/V0.3.0.1__get_flow_checkpoints.sql

+            CASE
+                WHEN i_latest_first THEN LC.process_start_time
+                END DESC,
+            CASE
+                WHEN NOT i_latest_first THEN LC.process_start_time
+                END ASC;


used the coalesce

salamonpavel added 4 commits November 20, 2024 11:39

tmp

6f37267

fix test

68e5622

tests

e6a65b3

ignore compatibility test

df7119d

ignore compatibility test

ce05796

salamonpavel marked this pull request as ready for review November 21, 2024 10:57

salamonpavel requested review from benedeki, lsulak, TebaleloS, Zejnilovic and dk1844 as code owners November 21, 2024 10:57

benedeki assigned salamonpavel Nov 22, 2024

benedeki changed the title ~~Feature/303 get flow checkpoints refactoring~~ #303: Get flow checkpoints refactoring Nov 22, 2024

benedeki reviewed Nov 22, 2024

View reviewed changes

salamonpavel and others added 10 commits November 22, 2024 14:15

Update database/README.md

52a9243

Co-authored-by: David Benedeki <[email protected]>

o_partitioning renamings

0ac613a

Merge remote-tracking branch 'origin/feature/303-get-flow-checkpoints…

e3d198c

…-refactoring' into feature/303-get-flow-checkpoints-refactoring

desc

e0f40a5

orderedCheckpointIds

353af66

removed needless prefix

0c520c5

refactor groupAndConvertItemsToCheckpointWithPartitioningDTOs

7e4513a

Update server/src/main/scala/za/co/absa/atum/server/model/CheckpointI…

30c0766

…temWithPartitioningFromDB.scala Co-authored-by: David Benedeki <[email protected]>

refactor fromItemsToCheckpointWithPartitioningDTO

93a64f0

Merge branch 'master' into feature/303-get-flow-checkpoints-refactoring

ba52d4e

benedeki requested a review from ABLL526 as a code owner November 22, 2024 21:01

switch order of actual and expected in asserts

b5122c8

benedeki previously approved these changes Nov 25, 2024

View reviewed changes

ABLL526 previously approved these changes Nov 25, 2024

View reviewed changes

lsulak reviewed Nov 25, 2024

View reviewed changes

explanatory comment added

728ee5d

salamonpavel dismissed stale reviews from ABLL526 and benedeki via 728ee5d November 26, 2024 08:11

salamonpavel added 2 commits November 26, 2024 10:18

i_latest_first param and conditional sorting

4d0175b

remove .bloop

877fac3

lsulak previously approved these changes Nov 26, 2024

View reviewed changes

explicit asc ordering

57c5bbe

salamonpavel dismissed lsulak’s stale review via 57c5bbe November 26, 2024 10:13

lsulak previously approved these changes Nov 26, 2024

View reviewed changes

benedeki requested changes Nov 26, 2024

View reviewed changes

use coalesce

12466a0

salamonpavel dismissed lsulak’s stale review via 12466a0 November 26, 2024 14:07

benedeki approved these changes Nov 26, 2024

View reviewed changes

salamonpavel merged commit 8603720 into master Nov 27, 2024
8 of 9 checks passed

salamonpavel deleted the feature/303-get-flow-checkpoints-refactoring branch November 27, 2024 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#303: Get flow checkpoints refactoring #304

#303: Get flow checkpoints refactoring #304

salamonpavel commented Nov 21, 2024 •

edited

Loading

salamonpavel commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

benedeki Nov 22, 2024

salamonpavel Nov 22, 2024

benedeki Nov 25, 2024

benedeki left a comment

ABLL526 left a comment

lsulak Nov 25, 2024

salamonpavel Nov 26, 2024

lsulak Nov 25, 2024 •

edited

Loading

lsulak Nov 25, 2024

salamonpavel Nov 26, 2024

salamonpavel Nov 26, 2024

lsulak left a comment •

edited

Loading

lsulak left a comment

benedeki Nov 26, 2024

salamonpavel Nov 26, 2024

benedeki Nov 26, 2024

benedeki Nov 26, 2024

benedeki Nov 26, 2024

salamonpavel Nov 26, 2024


		object CheckpointItemWithPartitioningFromDB {

		private def fromItemsToCheckpointWithPartitioningDTO(

#303: Get flow checkpoints refactoring #304

#303: Get flow checkpoints refactoring #304

Conversation

salamonpavel commented Nov 21, 2024 • edited Loading

salamonpavel commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 • edited Loading

JaCoCo model module code coverage report - scala 2.13.11

github-actions bot commented Nov 21, 2024 • edited Loading

JaCoCo agent module code coverage report - scala 2.13.11

github-actions bot commented Nov 21, 2024 • edited Loading

JaCoCo reader module code coverage report - scala 2.13.11

github-actions bot commented Nov 21, 2024 • edited Loading

JaCoCo server module code coverage report - scala 2.13.11

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedeki left a comment

Choose a reason for hiding this comment

ABLL526 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsulak Nov 25, 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

lsulak left a comment • edited Loading

Choose a reason for hiding this comment

lsulak 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

salamonpavel commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

lsulak Nov 25, 2024 •

edited

Loading

lsulak left a comment •

edited

Loading