You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/24 23:04:03 UTC
[GitHub] [superset] eric-briscoe opened a new pull request, #20179: Convert QueryAutoRefresh to TypeScript functional React component
eric-briscoe opened a new pull request, #20179:
URL: https://github.com/apache/superset/pull/20179
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
Convert QueryAutoRefresh to TypeScript functional React component
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR converts QueryAutoRefresh from es6 class component to TypeScript functional component as part of bigger effort to migrate frontend code to TypeScript and functional React components defined in: https://github.com/apache/superset/discussions/18388
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
There should be no visual change to the application
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Open SQL Lab / SQL Editor
2. Click the "RUN" button
3. Ensure Query runs and updates results with same behavior that existed prior to this PR
4. Test with both default behavior and asynchronous query execution on for a database
**Changes to http requests for getting Query statuses:**
If you watch the dev tools network tab you should see less network traffic as the execution now ensures only one outstanding http call to check query status is made at a time to avoid http response race conditions that can lead to incorrect application state and to reduce tandem network calls on slower connections. If the call fails, the offline state change will be triggered but the interval timer will continue attempting to reconnect and set user online and show results once the connection is restored. This can be tested by:
1. Opening the dev tools / Network / set the connection from "No Throttling" to "Offline", 2. Click the "Run" button as described in the initial test steps above
3. See the network failure and the UI will display an "Offline" pill (this is not new UI behavior)
4. Set the network back to "No Throttling" and wait a few seconds and the next interval will make a successful query status API call and you will see data results again.
QueryAutoRefresh was separated into two files to separate Redux knowledge from the simple functional component:
1. QueryAutoRefresh/index.ts - Simple functional component with no knowledge of Redux
3. QueryAutoRefresh/QueryAutoRefreshContainer.ts - Functional component container with connection to redux state that imports and contains QueryAutoRefresh/index.ts component
**IMPORTANT:**
When changing QueryAutoRefresh to a functional component the behavior of setInterval no longer worked correctly. This is a known issue with functional React components using setInterval can provide stale state / props in the interval callback creating undesired, hard to debug / fix behavior. A new utility hook (SqlLab/utils/useInterval.ts) was introduced in this PR to handle this based on this very helpful article by Dan Abramov (thank you!): https://overreacted.io/making-setinterval-declarative-with-react-hooks/
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [X] Has associated issue: https://github.com/apache/superset/discussions/18388
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881964835
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
Review Comment:
Cool, yeah I agree with runtime checks, too. This sounds great. The only part that reads a little confusing to me, understanding that `typeof` an array is an object, but I suppose currently we're allowing an object here as well, esp since we take Object.values before running the array method on it.
How do you feel about doing an `Array.isArray` check on queryList to be more specific on only allowing an array? And then I suppose we wouldn't need the Object.values, but could just do `queryList.some`. Does that still work 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881059352
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
Review Comment:
The typing in the function params for queryList is an array and not optional. If we want to rely on that, then do we need the second runtime check here for type? Asking only because if a js file is passing in the data, we're not guaranteed that the typing will match, so I'm wondering if that's the intent. We don't really have strong guidelines one way or the other, but it's a good conversation to have.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component [sc-48362]
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r898292765
##########
superset-frontend/src/SqlLab/utils/useInterval.ts:
##########
@@ -0,0 +1,47 @@
+/**
+ * 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.
+ */
+import { useEffect, useRef } from 'react';
+
+/*
+ * Functional components and setTimeout with useState do not play well
+ * and the setTimeout callback typically has stale state from a closure
+ * The useInterval function solves this issue.
+ * more info: https://overreacted.io/making-setinterval-declarative-with-react-hooks/
+ */
+function useInterval(callback: Function, delay: number | null): void {
Review Comment:
@hughhhh I'm thinking it may be dangerous to run the function continuously without any delay interval (or the minimum interval that the browser allows). If you forget to pass a delay, this could be problematic. I think @eric-briscoe has a good idea about doing nothing (returning a noop). WDYT?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881166798
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
Review Comment:
@eschutho This is definitely a great area for discussion! I usually try to make code runtime fault tolerant and verify that behavior in the unit tests. Because TypeScript is only doing static code analysis at compile time it really provides not actual safety for the runtime execution. TypeScript will catch a TON of places in code where the wrong type of value is passed, but it cannot guarantee null, undefined, invalid data ends up in redux or passed down on props at runtime. The associated unit tests failed when I passed a null or undefined value so I added the explicit check if the value is both truthy and object because `typeof null === 'object'`.
Also verifying the typeof is object helps avoid odd Object.values behavior if someone happened to pass a string instead of object this will make each character of a string a value in the response array. This portion of code is not performance sensitive where it is executing rapidly in a loop, or expensive to do these checks so I felt it was worth the check to make the code bomb proof. Pretty flexible on this if anyone has strong opinions otherwise but my go to is to try to ensure there is no way a function can produce an uncaught exception.
React rendering loops can be pretty unforgiving where one call like object.values(null) in render loop can lead you to white screen of doom 😮 Trust but verify I always say :)
Another approach I have used in the past is to add utility functions that can be imported like isArray, isObject, isString, etc so things like `if (queryList && typeof queryList === 'object') {` can be reduced to `if (isObject(queryList)) {`. Having those utilities can lead to consistent code practice for validating inputs and help eliminate potential runtime errors.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r882119566
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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.
+ */
+import { useState } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from 'src/SqlLab/utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
Review Comment:
So I looked into that a bit, and it's related to Scheduled Queries. The config/feature doesn't seem to be working as expected, but regardless, let's assume that it will create a query with a status of schedule, so I'd say yes, let's go ahead and add that value as an option.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881067583
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
+ q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL,
+ );
+ }
+ return shouldCheck;
+};
+
+function QueryAutoRefresh({
+ offline,
+ queries,
+ actions,
+ queriesLastUpdate,
+}: QueryAutoRefreshProps) {
+ // In case of an error / timeout requesting query execution state
+ // we set isOffline to true. the interval timer will continue to run
+ // and update status if connection succeeds later
+ const [isOffline, setIsOffline] = useState(offline);
+ // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order
+ // pendingRequest check ensures we only have one active http call to check for query statuses
+ const [pendingRequest, setPendingRequest] = useState(false);
+
+ useEffect(() => {
+ if (isOffline !== offline) {
+ actions?.setUserOffline(isOffline);
+ }
+ }, [isOffline, offline, actions]);
+
+ const refreshQueries = () => {
+ if (!pendingRequest && (shouldCheckForQueries(queries) || isOffline)) {
+ setPendingRequest(true);
+ SupersetClient.get({
+ endpoint: `/superset/queries/${
+ queriesLastUpdate - QUERY_UPDATE_BUFFER_MS
+ }`,
+ timeout: QUERY_TIMEOUT_LIMIT,
+ })
+ .then(({ json }) => {
+ if (json && Object.keys(json)?.length > 0) {
+ actions?.refreshQueries(json);
+ }
+ setIsOffline(false);
+ setPendingRequest(false);
+ })
+ .catch(() => {
+ setIsOffline(true);
+ setPendingRequest(false);
Review Comment:
nit, but maybe you can put setPendingRequests(false) into a finally event to dry it up a little.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881218764
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
Review Comment:
Great catch, has been updated to use includes
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r882179403
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
Review Comment:
@eschutho Your question here made me re-evaluate the code and realized that the confusion you are having is due to me marking the queryList arg as Query[] when it really is receiving a Dictionary of queries. I have pushed a commit that resolves this mismatch and also am now using lodash's isObject since we are already using lodash in the project and it provides all the type checking utils like isObject, isString, etc and we do not need to reinvent the wheel. Please take a look at the updated code and see if it makes more sense now. It was not doing an Object.keys on an array, it was receiving an object setup as a dictionary with query id as the key, but I treated the arg like an array when adding the TypeScript annotations which was a mistake. Nice catch!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component [sc-48362]
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r890161611
##########
superset-frontend/src/SqlLab/utils/useInterval.ts:
##########
@@ -0,0 +1,47 @@
+/**
+ * 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.
+ */
+import { useEffect, useRef } from 'react';
+
+/*
+ * Functional components and setTimeout with useState do not play well
+ * and the setTimeout callback typically has stale state from a closure
+ * The useInterval function solves this issue.
+ * more info: https://overreacted.io/making-setinterval-declarative-with-react-hooks/
+ */
+function useInterval(callback: Function, delay: number | null): void {
Review Comment:
nit: can we set delay as an optional and = to 0
```
function useInterval(callback: Function, delay: number = 0): void {
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881056658
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
Review Comment:
also, apologies, I just realized that this prob isn't your code, since it's a conversion to ts. :)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20179:
URL: https://github.com/apache/superset/pull/20179#issuecomment-1137451484
# [Codecov](https://codecov.io/gh/apache/superset/pull/20179?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20179](https://codecov.io/gh/apache/superset/pull/20179?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9888daa) into [master](https://codecov.io/gh/apache/superset/commit/7e9b85f76ca8cae38c38e11f857634216b1cd71c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7e9b85f) will **decrease** coverage by `0.00%`.
> The diff coverage is `63.41%`.
```diff
@@ Coverage Diff @@
## master #20179 +/- ##
==========================================
- Coverage 66.43% 66.42% -0.01%
==========================================
Files 1721 1723 +2
Lines 64571 64580 +9
Branches 6814 6814
==========================================
+ Hits 42895 42900 +5
- Misses 19942 19947 +5
+ Partials 1734 1733 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.30% <63.41%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...erset-frontend/src/SqlLab/components/App/index.jsx](https://codecov.io/gh/apache/superset/pull/20179/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FwcC9pbmRleC5qc3g=) | `55.55% <ø> (ø)` | |
| [...nts/QueryAutoRefresh/QueryAutoRefreshContainer.tsx](https://codecov.io/gh/apache/superset/pull/20179/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5QXV0b1JlZnJlc2gvUXVlcnlBdXRvUmVmcmVzaENvbnRhaW5lci50c3g=) | `0.00% <0.00%> (ø)` | |
| [...d/src/SqlLab/components/QueryAutoRefresh/index.tsx](https://codecov.io/gh/apache/superset/pull/20179/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5QXV0b1JlZnJlc2gvaW5kZXgudHN4) | `65.38% <65.38%> (ø)` | |
| [superset-frontend/src/SqlLab/utils/useInterval.ts](https://codecov.io/gh/apache/superset/pull/20179/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi91dGlscy91c2VJbnRlcnZhbC50cw==) | `66.66% <66.66%> (ø)` | |
| [superset-frontend/src/SqlLab/fixtures.ts](https://codecov.io/gh/apache/superset/pull/20179/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/superset/pull/20179/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `60.11% <0.00%> (-0.29%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20179?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20179?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [7e9b85f...9888daa](https://codecov.io/gh/apache/superset/pull/20179?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r882095406
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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.
+ */
+import { useState } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from 'src/SqlLab/utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
Review Comment:
Oh yeah I remember this, it is a bit confusing because I think some of these states only occur in the frontend and never get passed to the backend. I once broke this in superset, let me check to see what I needed to do to fix
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r882181920
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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.
+ */
+import { useState } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from 'src/SqlLab/utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
Review Comment:
@AAfghahi @eschutho Thank you for the context. I just pushed up a commit that adds QueryState.SCHEDULED to runningQueryStateList
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r882097375
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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.
+ */
+import { useState } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from 'src/SqlLab/utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
Review Comment:
I think this is all of them:
https://github.com/apache/superset/blob/master/superset-frontend/src/SqlLab/components/QueryTable/index.tsx#L117
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881894738
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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.
+ */
+import { useState } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from 'src/SqlLab/utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
Review Comment:
@AAfghahi Agree it would be better to create a typescript enum for this in the types.ts file where Query is defined and use that.
`/ / Possible states of a query object for processing on the server
export enum QueryState {
STARTED = 'started',
STOPPED = 'stopped',
FAILED = 'failed',
PENDING = 'pending',
RUNNING = 'running',
SCHEDULED = 'scheduled',
SUCCESS = 'success',
FETCHING = 'fetching',
TIMED_OUT = 'timed_out',
}
// Indicates a Query's state is still processing
export const runningQueryStateList: QueryState[] = [
QueryState.RUNNING,
QueryState.STARTED,
QueryState.PENDING,
QueryState.FETCHING,
];
// Indictes a Query's state has completed processing regardless of success / failure
export const concludedQueryStateList: QueryState[] = [
QueryState.STOPPED,
QueryState.FAILED,
QueryState.SUCCESS,
QueryState.TIMED_OUT,
];
// check if a query is in a running state
export const isQueryRunning = (q: Query): boolean => runningQueryStateList.includes(q?.state);
`
I have this setup and working on local but saw an additional query state value 'scheduled' in types.ts that seems like it should be included in the array used by isQueryRunning. Anyone have context if 'scheduled' should be added to current list of ['running', 'started', 'pending', 'fetching']?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881055743
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
Review Comment:
nit, but `['running', 'started', 'pending', 'fetching'].includes(q?.state)` may be just a bit more terse.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho merged pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component [sc-48362]
Posted by GitBox <gi...@apache.org>.
eschutho merged PR #20179:
URL: https://github.com/apache/superset/pull/20179
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881066632
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
+ q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL,
+ );
+ }
+ return shouldCheck;
+};
+
+function QueryAutoRefresh({
+ offline,
+ queries,
+ actions,
+ queriesLastUpdate,
+}: QueryAutoRefreshProps) {
+ // In case of an error / timeout requesting query execution state
+ // we set isOffline to true. the interval timer will continue to run
+ // and update status if connection succeeds later
+ const [isOffline, setIsOffline] = useState(offline);
+ // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order
+ // pendingRequest check ensures we only have one active http call to check for query statuses
+ const [pendingRequest, setPendingRequest] = useState(false);
+
+ useEffect(() => {
+ if (isOffline !== offline) {
+ actions?.setUserOffline(isOffline);
Review Comment:
Is there a benefit to keeping the local state and the parent state in this way vs having the parent be the source of truth and only using the props for current state?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on PR #20179:
URL: https://github.com/apache/superset/pull/20179#issuecomment-1137942451
@eric-briscoe do you have the pre-commit hook installed? We have it in a makefile. The command is `make pre-commit`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881773386
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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.
+ */
+import { useState } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from 'src/SqlLab/utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
Review Comment:
Would this be better as an enum?
##########
superset-frontend/src/SqlLab/fixtures.ts:
##########
@@ -512,7 +513,86 @@ export const failedQueryWithErrors = {
tempTable: '',
};
-export const runningQuery = {
+const baseQuery: Query = {
Review Comment:
crazy that this hasn't existed 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881175107
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
+ q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL,
+ );
+ }
+ return shouldCheck;
+};
+
+function QueryAutoRefresh({
+ offline,
+ queries,
+ actions,
+ queriesLastUpdate,
+}: QueryAutoRefreshProps) {
+ // In case of an error / timeout requesting query execution state
+ // we set isOffline to true. the interval timer will continue to run
+ // and update status if connection succeeds later
+ const [isOffline, setIsOffline] = useState(offline);
+ // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order
+ // pendingRequest check ensures we only have one active http call to check for query statuses
+ const [pendingRequest, setPendingRequest] = useState(false);
+
+ useEffect(() => {
+ if (isOffline !== offline) {
+ actions?.setUserOffline(isOffline);
+ }
+ }, [isOffline, offline, actions]);
+
+ const refreshQueries = () => {
+ if (!pendingRequest && (shouldCheckForQueries(queries) || isOffline)) {
+ setPendingRequest(true);
+ SupersetClient.get({
+ endpoint: `/superset/queries/${
+ queriesLastUpdate - QUERY_UPDATE_BUFFER_MS
+ }`,
+ timeout: QUERY_TIMEOUT_LIMIT,
+ })
+ .then(({ json }) => {
+ if (json && Object.keys(json)?.length > 0) {
+ actions?.refreshQueries(json);
+ }
+ setIsOffline(false);
+ setPendingRequest(false);
+ })
+ .catch(() => {
+ setIsOffline(true);
+ setPendingRequest(false);
Review Comment:
Great catch! Will make change
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881219820
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
+ q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL,
+ );
+ }
+ return shouldCheck;
+};
+
+function QueryAutoRefresh({
+ offline,
+ queries,
+ actions,
+ queriesLastUpdate,
+}: QueryAutoRefreshProps) {
+ // In case of an error / timeout requesting query execution state
+ // we set isOffline to true. the interval timer will continue to run
+ // and update status if connection succeeds later
+ const [isOffline, setIsOffline] = useState(offline);
+ // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order
+ // pendingRequest check ensures we only have one active http call to check for query statuses
+ const [pendingRequest, setPendingRequest] = useState(false);
+
+ useEffect(() => {
+ if (isOffline !== offline) {
+ actions?.setUserOffline(isOffline);
+ }
+ }, [isOffline, offline, actions]);
+
+ const refreshQueries = () => {
+ if (!pendingRequest && (shouldCheckForQueries(queries) || isOffline)) {
+ setPendingRequest(true);
+ SupersetClient.get({
+ endpoint: `/superset/queries/${
+ queriesLastUpdate - QUERY_UPDATE_BUFFER_MS
+ }`,
+ timeout: QUERY_TIMEOUT_LIMIT,
+ })
+ .then(({ json }) => {
+ if (json && Object.keys(json)?.length > 0) {
+ actions?.refreshQueries(json);
Review Comment:
I was not sure why it was originally written this way so I had left it alone. I removed the check and found no side affect / change to behavior and updated to remove the Object.keys check
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881219933
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
+ q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL,
+ );
+ }
+ return shouldCheck;
+};
+
+function QueryAutoRefresh({
+ offline,
+ queries,
+ actions,
+ queriesLastUpdate,
+}: QueryAutoRefreshProps) {
+ // In case of an error / timeout requesting query execution state
+ // we set isOffline to true. the interval timer will continue to run
+ // and update status if connection succeeds later
+ const [isOffline, setIsOffline] = useState(offline);
+ // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order
+ // pendingRequest check ensures we only have one active http call to check for query statuses
+ const [pendingRequest, setPendingRequest] = useState(false);
+
+ useEffect(() => {
+ if (isOffline !== offline) {
+ actions?.setUserOffline(isOffline);
+ }
+ }, [isOffline, offline, actions]);
+
+ const refreshQueries = () => {
+ if (!pendingRequest && (shouldCheckForQueries(queries) || isOffline)) {
+ setPendingRequest(true);
+ SupersetClient.get({
+ endpoint: `/superset/queries/${
+ queriesLastUpdate - QUERY_UPDATE_BUFFER_MS
+ }`,
+ timeout: QUERY_TIMEOUT_LIMIT,
+ })
+ .then(({ json }) => {
+ if (json && Object.keys(json)?.length > 0) {
+ actions?.refreshQueries(json);
+ }
+ setIsOffline(false);
+ setPendingRequest(false);
+ })
+ .catch(() => {
+ setIsOffline(true);
+ setPendingRequest(false);
Review Comment:
Updated in new commit
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881219086
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
+ q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL,
+ );
+ }
+ return shouldCheck;
+};
+
+function QueryAutoRefresh({
+ offline,
+ queries,
+ actions,
+ queriesLastUpdate,
+}: QueryAutoRefreshProps) {
+ // In case of an error / timeout requesting query execution state
+ // we set isOffline to true. the interval timer will continue to run
+ // and update status if connection succeeds later
+ const [isOffline, setIsOffline] = useState(offline);
+ // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order
+ // pendingRequest check ensures we only have one active http call to check for query statuses
+ const [pendingRequest, setPendingRequest] = useState(false);
+
+ useEffect(() => {
+ if (isOffline !== offline) {
+ actions?.setUserOffline(isOffline);
Review Comment:
Dug into this more and was able to completely remove offline prop and state tracking with no affect to behavior
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881067216
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
+ q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL,
+ );
+ }
+ return shouldCheck;
+};
+
+function QueryAutoRefresh({
+ offline,
+ queries,
+ actions,
+ queriesLastUpdate,
+}: QueryAutoRefreshProps) {
+ // In case of an error / timeout requesting query execution state
+ // we set isOffline to true. the interval timer will continue to run
+ // and update status if connection succeeds later
+ const [isOffline, setIsOffline] = useState(offline);
+ // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order
+ // pendingRequest check ensures we only have one active http call to check for query statuses
+ const [pendingRequest, setPendingRequest] = useState(false);
+
+ useEffect(() => {
+ if (isOffline !== offline) {
+ actions?.setUserOffline(isOffline);
+ }
+ }, [isOffline, offline, actions]);
+
+ const refreshQueries = () => {
+ if (!pendingRequest && (shouldCheckForQueries(queries) || isOffline)) {
+ setPendingRequest(true);
+ SupersetClient.get({
+ endpoint: `/superset/queries/${
+ queriesLastUpdate - QUERY_UPDATE_BUFFER_MS
+ }`,
+ timeout: QUERY_TIMEOUT_LIMIT,
+ })
+ .then(({ json }) => {
+ if (json && Object.keys(json)?.length > 0) {
+ actions?.refreshQueries(json);
Review Comment:
is there ever a case where you want to clear out all of the queries, e.g., the json comes back blank?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r882182096
##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+import { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+ (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+ (alteredQueries: any): any;
+}
+
+interface Actions {
+ setUserOffline: UserOfflineFunc;
+ refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+ offline: boolean;
+ queries: Query[];
+ actions: Actions;
+ queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+ ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+ let shouldCheck = false;
+ // if there are started or running queries, this method should return true
+ const now = Date.now();
+ if (queryList && typeof queryList === 'object') {
+ shouldCheck = Object.values(queryList).some(
Review Comment:
Oh, ya, that makes more sense. 👍 :)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org