pierrejeambrun commented on code in PR #45977:
URL: https://github.com/apache/airflow/pull/45977#discussion_r1935767230
##########
airflow/ui/src/queries/usePatchDagRun.ts:
##########
@@ -45,7 +48,12 @@ export const usePatchDagRun = ({
const queryClient = useQueryClient();
const onSuccessFn = async () => {
- const queryKeys = [UseDagRunServiceGetDagRunKeyFn({ dagId, dagRunId }),
[useDagRunServiceGetDagRunsKey]];
+ const queryKeys = [
+ UseDagRunServiceGetDagRunKeyFn({ dagId, dagRunId }),
+ [useDagRunServiceGetDagRunsKey],
+ [useTaskInstanceServiceGetTaskInstancesKey, { dagId, dagRunId }],
Review Comment:
I think it's always better to invalidate if requests need to be invalidated
and not rely on possible auto-refresh to do that for us. So I would say this is
still needed.
> The change is about marking the task as success/failure. I don't think
useClearRun is invalidating the cache here
That's because I was testing the PatchDagRun through the `ClearRun` feature.
You are right, using the markas is simpler for testing.
I think I got confused because if you do not `edit` the dagrun `note` when
clearing, `patchDagRun` will not be called.
I tested again yes it's working.
> UseTaskInstanceServiceGetTaskInstancesKeyFn({ dagId, dagRunId }, [{ dagId,
dagRunId }]) and [useTaskInstanceServiceGetTaskInstancesKey,{ dagId, dagRunId
}] in PR construct the same value. I verified it by below statement in
onSuccess.
Yes but we should still use the auto-generated code interface. If for some
reason the underlying generated code change the way they implement
`UseTaskInstanceServiceGetTaskInstancesKeyFn` things will start breaking. (For
instance if we upgrade the version of `openapi-rq`)
> Maybe you can also update the call to
useTaskInstanceServiceGetTaskInstancesKey in useClearRun. I don't think we
should be clearing all TI calls.
For usePatchTaskInstance though, we need to clear per dagId. (because of
past/future we need to not be specific on the 'dagRun').
I still think we should do that.
##########
airflow/ui/src/queries/usePatchDagRun.ts:
##########
@@ -45,7 +48,12 @@ export const usePatchDagRun = ({
const queryClient = useQueryClient();
const onSuccessFn = async () => {
- const queryKeys = [UseDagRunServiceGetDagRunKeyFn({ dagId, dagRunId }),
[useDagRunServiceGetDagRunsKey]];
+ const queryKeys = [
+ UseDagRunServiceGetDagRunKeyFn({ dagId, dagRunId }),
+ [useDagRunServiceGetDagRunsKey],
+ [useTaskInstanceServiceGetTaskInstancesKey, { dagId, dagRunId }],
Review Comment:
Thanks for your response.
I think it's always better to invalidate if requests need to be invalidated
and not rely on possible auto-refresh to do that for us. So I would say this is
still needed.
> The change is about marking the task as success/failure. I don't think
useClearRun is invalidating the cache here
That's because I was testing the PatchDagRun through the `ClearRun` feature.
You are right, using the markas is simpler for testing.
I think I got confused because if you do not `edit` the dagrun `note` when
clearing, `patchDagRun` will not be called.
I tested again yes it's working.
> UseTaskInstanceServiceGetTaskInstancesKeyFn({ dagId, dagRunId }, [{ dagId,
dagRunId }]) and [useTaskInstanceServiceGetTaskInstancesKey,{ dagId, dagRunId
}] in PR construct the same value. I verified it by below statement in
onSuccess.
Yes but we should still use the auto-generated code interface. If for some
reason the underlying generated code change the way they implement
`UseTaskInstanceServiceGetTaskInstancesKeyFn` things will start breaking. (For
instance if we upgrade the version of `openapi-rq`)
> Maybe you can also update the call to
useTaskInstanceServiceGetTaskInstancesKey in useClearRun. I don't think we
should be clearing all TI calls.
For usePatchTaskInstance though, we need to clear per dagId. (because of
past/future we need to not be specific on the 'dagRun').
I still think we should do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
