Re: [PR] Add autorefresh to dags list page [airflow]
bbovenzi merged PR #46326: URL: https://github.com/apache/airflow/pull/46326 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add autorefresh to dags list page [airflow]
bbovenzi commented on code in PR #46326:
URL: https://github.com/apache/airflow/pull/46326#discussion_r1937658152
##
airflow/ui/src/components/TogglePause.tsx:
##
@@ -42,12 +43,19 @@ export const TogglePause = ({ dagDisplayName, dagId,
isPaused, skipConfirm }: Pr
const { onClose, onOpen, open } = useDisclosure();
const onSuccess = async () => {
-await queryClient.invalidateQueries({
- queryKey: [useDagServiceGetDagsKey],
-});
+// We can just update dag details
+
queryClient.setQueryData(UseDagServiceGetDagDetailsKeyFn({
dagId }), (oldData) =>
+ oldData
+? {
+...oldData,
+is_paused: !oldData.is_paused,
+ }
+: undefined,
+);
Review Comment:
Ok I'll drop it
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add autorefresh to dags list page [airflow]
pierrejeambrun commented on code in PR #46326:
URL: https://github.com/apache/airflow/pull/46326#discussion_r1937641504
##
airflow/ui/src/components/TogglePause.tsx:
##
@@ -42,12 +43,19 @@ export const TogglePause = ({ dagDisplayName, dagId,
isPaused, skipConfirm }: Pr
const { onClose, onOpen, open } = useDisclosure();
const onSuccess = async () => {
-await queryClient.invalidateQueries({
- queryKey: [useDagServiceGetDagsKey],
-});
+// We can just update dag details
+
queryClient.setQueryData(UseDagServiceGetDagDetailsKeyFn({
dagId }), (oldData) =>
+ oldData
+? {
+...oldData,
+is_paused: !oldData.is_paused,
+ }
+: undefined,
+);
Review Comment:
I'm not super fan of this.
If we add some business logic and side effect on pausing / unpausing a dag
in the backend, this will start failing. (data will be de-synced)
We are unnecessary coupling the back and the front on that part. I think an
extra GET is really not that expensive and worth the trouble. (if we generalize
that pattern we might end up with problem, or extra maintenance cost. As long
as performance is not an issue, I wouldn't try to optimize this)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add autorefresh to dags list page [airflow]
pierrejeambrun commented on code in PR #46326:
URL: https://github.com/apache/airflow/pull/46326#discussion_r1937641504
##
airflow/ui/src/components/TogglePause.tsx:
##
@@ -42,12 +43,19 @@ export const TogglePause = ({ dagDisplayName, dagId,
isPaused, skipConfirm }: Pr
const { onClose, onOpen, open } = useDisclosure();
const onSuccess = async () => {
-await queryClient.invalidateQueries({
- queryKey: [useDagServiceGetDagsKey],
-});
+// We can just update dag details
+
queryClient.setQueryData(UseDagServiceGetDagDetailsKeyFn({
dagId }), (oldData) =>
+ oldData
+? {
+...oldData,
+is_paused: !oldData.is_paused,
+ }
+: undefined,
+);
Review Comment:
I'm not super fan of this.
If we add some business logic and side effect on pausing / unpausing a dag
in the backend, this will start failing.
We are unnecessary coupling the back and the front on that part. I think an
extra GET is really not that expensive and worth the trouble. (if we generalize
that pattern we might end up with issues)
##
airflow/ui/src/components/TogglePause.tsx:
##
@@ -42,12 +43,19 @@ export const TogglePause = ({ dagDisplayName, dagId,
isPaused, skipConfirm }: Pr
const { onClose, onOpen, open } = useDisclosure();
const onSuccess = async () => {
-await queryClient.invalidateQueries({
- queryKey: [useDagServiceGetDagsKey],
-});
+// We can just update dag details
+
queryClient.setQueryData(UseDagServiceGetDagDetailsKeyFn({
dagId }), (oldData) =>
+ oldData
+? {
+...oldData,
+is_paused: !oldData.is_paused,
+ }
+: undefined,
+);
Review Comment:
I'm not super fan of this.
If we add some business logic and side effect on pausing / unpausing a dag
in the backend, this will start failing. (data will be de-synced)
We are unnecessary coupling the back and the front on that part. I think an
extra GET is really not that expensive and worth the trouble. (if we generalize
that pattern we might end up with issues)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add autorefresh to dags list page [airflow]
pierrejeambrun commented on code in PR #46326:
URL: https://github.com/apache/airflow/pull/46326#discussion_r1937641504
##
airflow/ui/src/components/TogglePause.tsx:
##
@@ -42,12 +43,19 @@ export const TogglePause = ({ dagDisplayName, dagId,
isPaused, skipConfirm }: Pr
const { onClose, onOpen, open } = useDisclosure();
const onSuccess = async () => {
-await queryClient.invalidateQueries({
- queryKey: [useDagServiceGetDagsKey],
-});
+// We can just update dag details
+
queryClient.setQueryData(UseDagServiceGetDagDetailsKeyFn({
dagId }), (oldData) =>
+ oldData
+? {
+...oldData,
+is_paused: !oldData.is_paused,
+ }
+: undefined,
+);
Review Comment:
I'm not super fan of this.
If we add some business logic and side effect on pausing / unpausing a dag.
That will start failing.
We are unnecessary coupling the back and the front on that part. I think an
extra GET is really not that expensive and worth the trouble. (if we generalize
that pattern we might end up with issues)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
