You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/06/16 21:10:07 UTC
[GitHub] [airflow] bbovenzi opened a new pull request, #24509: Migrate jsx files that affect run/task selection to tsx
bbovenzi opened a new pull request, #24509:
URL: https://github.com/apache/airflow/pull/24509
In the Grid view, selecting a task or run can be very slow for large DAGs. This is because of some slow querying. I am about to fix that. But I figured it would be safer to make a big change if all the related files were in typescript so issues are caught early.
Partial solution to: https://github.com/apache/airflow/issues/24350
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r901994553
##########
airflow/www/static/js/grid/components/InstanceTooltip.tsx:
##########
@@ -23,25 +23,33 @@ import { Box, Text } from '@chakra-ui/react';
import { finalStatesMap } from '../../utils';
import { formatDuration, getDuration } from '../../datetime_utils';
import Time from './Time';
+import type { TaskInstance, Task } from '../types';
-const InstanceTooltip = ({
+interface Props {
+ group: Task;
+ instance: TaskInstance;
+}
+
+const InstanceTooltip: React.FC<Props> = ({
Review Comment:
Interesting. You're right. As of React 18, it doesn't make sense anymore. I'll update 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r900132778
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
Review Comment:
Right, DagRun is only the first four. a Task Instance can have null, fixed.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi merged pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #24509:
URL: https://github.com/apache/airflow/pull/24509
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r901918799
##########
airflow/www/static/js/grid/components/InstanceTooltip.tsx:
##########
@@ -23,25 +23,33 @@ import { Box, Text } from '@chakra-ui/react';
import { finalStatesMap } from '../../utils';
import { formatDuration, getDuration } from '../../datetime_utils';
import Time from './Time';
+import type { TaskInstance, Task } from '../types';
-const InstanceTooltip = ({
+interface Props {
+ group: Task;
+ instance: TaskInstance;
+}
+
+const InstanceTooltip: React.FC<Props> = ({
Review Comment:
Any reason we favor React.FC over:
```
const MyComponent = ({}: Props): JSX.Element => {}
```
React.FC was removed in CRA:
https://github.com/facebook/create-react-app/pull/8177
React.FC seems discouraged:
https://github.com/typescript-cheatsheets/react#function-components
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
+
+type TaskState = RunState
+| 'removed'
+| 'scheduled'
+| 'shutdown'
+| 'restarting'
+| 'up_for_retry'
+| 'up_for_reschedule'
+| 'upstream_failed'
+| 'skipped'
+| 'sensing'
+| 'deferred'
+| null;
+
+interface DagRun {
+ runId: string;
+ runType: 'manual' | 'backfill' | 'scheduled';
+ state: RunState;
+ executionDate: string;
+ dataIntervalStart: string;
+ dataIntervalEnd: string;
+ startDate: string;
+ endDate: string;
+}
+
+interface GridTaskInstance {
+ runId: string;
+ taskId: string;
+ startDate: string | null;
+ endDate: string | null;
+ state: TaskState;
+ mappedStates?: {
+ [key: string]: number;
Review Comment:
You can do something like:
```
[key in TaskState]: number;
```
But the `| null` is an issue here. :(
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24509:
URL: https://github.com/apache/airflow/pull/24509#issuecomment-1158671987
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r899901464
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
+
+type TaskState = RunState
+| 'removed'
+| 'scheduled'
+| 'shutdown'
+| 'restarting'
+| 'up_for_retry'
+| 'up_for_reschedule'
+| 'upstream_failed'
+| 'skipped'
+| 'sensing'
+| 'deferred'
+| null;
+
+interface DagRun {
+ runId: string;
+ runType: 'manual' | 'backfill' | 'scheduled';
+ state: RunState;
+ executionDate: string;
+ dataIntervalStart: string;
+ dataIntervalEnd: string;
+ startDate: string;
+ endDate: string;
+}
+
+interface GridTaskInstance {
+ runId: string;
+ taskId: string;
+ startDate: string | null;
+ endDate: string | null;
+ state: TaskState;
+ mappedStates?: {
+ [key: string]: number;
Review Comment:
```suggestion
[key: TaskState]: number;
```
If that is allowed
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r899901183
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
+
+type TaskState = RunState
+| 'removed'
+| 'scheduled'
+| 'shutdown'
+| 'restarting'
+| 'up_for_retry'
+| 'up_for_reschedule'
+| 'upstream_failed'
+| 'skipped'
+| 'sensing'
+| 'deferred'
+| null;
+
+interface DagRun {
+ runId: string;
+ runType: 'manual' | 'backfill' | 'scheduled';
+ state: RunState;
+ executionDate: string;
+ dataIntervalStart: string;
+ dataIntervalEnd: string;
+ startDate: string;
+ endDate: string;
+}
+
+interface GridTaskInstance {
Review Comment:
Why the `Grid` prefix?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r900132282
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
+
+type TaskState = RunState
+| 'removed'
+| 'scheduled'
+| 'shutdown'
+| 'restarting'
+| 'up_for_retry'
+| 'up_for_reschedule'
+| 'upstream_failed'
+| 'skipped'
+| 'sensing'
+| 'deferred'
+| null;
+
+interface DagRun {
+ runId: string;
+ runType: 'manual' | 'backfill' | 'scheduled';
+ state: RunState;
+ executionDate: string;
+ dataIntervalStart: string;
+ dataIntervalEnd: string;
+ startDate: string;
+ endDate: string;
+}
+
+interface GridTaskInstance {
+ runId: string;
+ taskId: string;
+ startDate: string | null;
+ endDate: string | null;
+ state: TaskState;
+ mappedStates?: {
+ [key: string]: number;
Review Comment:
Unfortunately not to my knowledge.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r899899706
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
Review Comment:
Do we want both no_status and empty string here?
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
Review Comment:
And why empty string and not null?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #24509: Migrate jsx files that affect run/task selection to tsx
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #24509:
URL: https://github.com/apache/airflow/pull/24509#discussion_r899900756
##########
airflow/www/static/js/grid/types/index.ts:
##########
@@ -0,0 +1,73 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type RunState = 'success' | 'running' | 'queued' | 'failed' | 'no_status' | '';
+
+type TaskState = RunState
+| 'removed'
+| 'scheduled'
+| 'shutdown'
+| 'restarting'
+| 'up_for_retry'
+| 'up_for_reschedule'
+| 'upstream_failed'
+| 'skipped'
+| 'sensing'
+| 'deferred'
+| null;
+
+interface DagRun {
+ runId: string;
+ runType: 'manual' | 'backfill' | 'scheduled';
+ state: RunState;
+ executionDate: string;
+ dataIntervalStart: string;
+ dataIntervalEnd: string;
+ startDate: string;
+ endDate: string;
Review Comment:
```suggestion
startDate: string | null;
endDate: string | null;
```
I think, as these can be empty if the run hasn't started yet.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org