get flow checkpoints endpoint v2: 233 #264
Conversation
TebaleloS
added
DB
# Conflicts: # server/src/main/scala/za/co/absa/atum/server/api/controller/BaseController.scala # server/src/main/scala/za/co/absa/atum/server/api/http/Routes.scala
| sealed trait PaginatedResult[R] { | ||
| def data: Seq[R] | ||
| } | ||
|
|
||
| object PaginatedResult { | ||
|
|
||
| case class ResultHasMore[R](data: Seq[R]) extends PaginatedResult[R] | ||
|
|
||
| object ResultHasMore { |
There was a problem hiding this comment.
Remove these objects. We do not need encoders/decoders for PaginatedResults, we need it only for PaginatedResponse.
Release Notes
|
| @@ -14,10 +14,12 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| CREATE OR REPLACE FUNCTION flows.get_flow_checkpoints( | |||
| IN i_partitioning_of_flow JSONB, | |||
| -- Function: flows.get_flow_checkpoints_v2(4) | |||
There was a problem hiding this comment.
It should be just bellow. It's then part of the function code, can be read within the DB.
| IN i_partitioning_of_flow JSONB, | ||
| -- Function: flows.get_flow_checkpoints_v2(4) | ||
| CREATE OR REPLACE FUNCTION flows.get_flow_checkpoints_v2( | ||
| IN i_flow_id BIGINT, |
There was a problem hiding this comment.
And even more importantly try to keep the order "natural" - the more important parameters first.
| @@ -66,53 +70,60 @@ $$ | |||
| -- measured_columns - measure columns associated with a given checkpoint | |||
| -- measurement_value - measurement details associated with a given checkpoint | |||
| -- checkpoint_time - time | |||
| -- has_more - flag indicating whether there are more checkpoints available | |||
There was a problem hiding this comment.
| -- has_more - flag indicating whether there are more checkpoints available | |
| -- has_more - flag indicating whether there are more checkpoints available, always `false` if `i_limit` is NULL |
| IN i_partitioning_of_flow JSONB, | ||
| -- Function: flows.get_flow_checkpoints_v2(4) | ||
| CREATE OR REPLACE FUNCTION flows.get_flow_checkpoints_v2( | ||
| IN i_flow_id BIGINT, | ||
| IN i_limit INT DEFAULT 5, |
There was a problem hiding this comment.
Would rename to i_checkpoint_limit, it's more explanatory.
| CREATE OR REPLACE FUNCTION flows.get_flow_checkpoints( | ||
| IN i_partitioning_of_flow JSONB, | ||
| -- Function: flows.get_flow_checkpoints_v2(4) | ||
| CREATE OR REPLACE FUNCTION flows.get_flow_checkpoints_v2( |
There was a problem hiding this comment.
No need to be marked with v2
| CREATE OR REPLACE FUNCTION flows.get_flow_checkpoints_v2( | |
| CREATE OR REPLACE FUNCTION flows.get_flow_checkpoints( |
It has consequences bellow
| BEGIN | ||
| _fk_partitioning = runs._get_id_partitioning(i_partitioning_of_flow); | ||
| -- Execute the query to retrieve checkpoints flow and their associated measurements | ||
| RETURN QUERY |
There was a problem hiding this comment.
The query is wrong.
The limit should be applied to number of checkpoints not, the measurements eventually.
I don't think it's possible to solve without a WITH statement.
There was a problem hiding this comment.
I will fix it; I will use the agreed method. That I think will fix a lot of things that could go wrong in the procedure.
| ON M.fk_measure_definition = MD.id_measure_definition | ||
| WHERE PF.fk_flow = i_flow_id | ||
| AND (i_checkpoint_name IS NULL OR CP.checkpoint_name = i_checkpoint_name) | ||
| ORDER BY CP.process_start_time, |
There was a problem hiding this comment.
Ordering should be switched. See why here.
TebaleloS
removed
the
work in progress
# Conflicts: # server/src/main/scala/za/co/absa/atum/server/Main.scala # server/src/main/scala/za/co/absa/atum/server/api/controller/BaseController.scala # server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala # server/src/main/scala/za/co/absa/atum/server/api/http/Routes.scala # server/src/test/scala/za/co/absa/atum/server/api/TestData.scala
# Conflicts: # server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala # server/src/main/scala/za/co/absa/atum/server/api/http/Routes.scala
| IF _fk_partitioning IS NULL THEN | ||
| status := 41; | ||
| status_text := 'Partitioning not found'; | ||
| -- Check if the flow exists |
There was a problem hiding this comment.
Just documentation:
First instinct to check if flows exists, is to go to the flows table. But every flow has at least one partitioning, so at least one record here. And this table is actually used in further queries unlike the flows table, so caching advantage. Great solution. 👏
But as it's a less intuitive solution, please add to the comment why it is done and why it works.
database/src/main/postgres/flows/V0.2.0.57__get_flow_checkpoints.sql
Outdated
Show resolved
Hide resolved
database/src/main/postgres/flows/V0.2.0.57__get_flow_checkpoints.sql
Outdated
Show resolved
Hide resolved
| @@ -124,7 +132,8 @@ trait Routes extends Endpoints with ServerOptions { | |||
| // getPartitioningEndpointV2, | |||
| // getPartitioningMeasuresEndpointV2, | |||
| // getFlowPartitioningsEndpointV2, | |||
| // getPartitioningMainFlowEndpointV2 | |||
| // getPartitioningMainFlowEndpointV2, | |||
| getFlowCheckpointsEndpointV2, | |||
There was a problem hiding this comment.
This needs to be also commented.
…ts.sql Co-authored-by: David Benedeki <14905969+benedeki@users.noreply.github.com>
…ts.sql Co-authored-by: David Benedeki <14905969+benedeki@users.noreply.github.com>
| status_text := 'Partitioning not found'; | ||
| -- Check if the flow exists by querying the partitioning_to_flow table. | ||
| -- Rationale: | ||
| -- This table is preferred over the flows table because: |
Release notes: