Skip to content

Remove extraneous App Session watch logic#40092

Merged
Joerger merged 3 commits intomasterfrom
joerger/remove-web-session-wait-logic
Apr 17, 2024
Merged

Remove extraneous App Session watch logic#40092
Joerger merged 3 commits intomasterfrom
joerger/remove-web-session-wait-logic

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Apr 1, 2024

Changelog: Improve efficiency of App Access session creation

This PR removes extraneous App Session and Snowflake Session client-side watcher logic.

In the original App Access implementation, it was possible for an App session to be created without it propagating to the Proxy's cache before the user attempted to use it. This race condition was handled with an Auth client watcher immediately after calling CreateAppSession. IIUC, the logic here was that if the app session propagated to the client watcher, it should have propagated to the proxy cache shortly before, thus avoiding the race condition.

This racy condition is no longer possible with the introduction of the cache fallback mechanism for web session in v15. The Proxy will simply check Auth itself if it doesn't find the web session in the cache.

In fact, in v15 this watcher wasn't even doing anything since the GetAppSession call at the start of WaitForAppSession would immediately fall back to the auth cache, and then the auth backend.

Note: as the above fix wasn't added until v15, this change will not be backported to avoid compatibility issues with old Proxy and Snowflake DB services.

@Joerger Joerger requested a review from fspmarshall April 1, 2024 19:39
@github-actions github-actions Bot added database-access Database access related issues and PRs machine-id size/sm labels Apr 1, 2024
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Apr 1, 2024

@fspmarshall Creating sessions in the WebUI with this PR results in a warning log:

2024-04-01T12:43:56-07:00 WARN [PROXY:1:C] Cache was forced to load session app_session/3a755c72d35cfd750e4bf012fcd5ffe92f808b7fabc39e3f8eb13bbed19fdefb from upstream. Frequent occurrence may indicate sync/perf issues. cache/cache.go:2371

In the Auth service, I'm also seeing multiple logs back to back, since we only claim a read lock:

2024-04-01T12:43:56-07:00 WARN [AUTH:2:CA] Cache was forced to load session app_session/3a755c72d35cfd750e4bf012fcd5ffe92f808b7fabc39e3f8eb13bbed19fdefb from upstream. Frequent occurrence may indicate sync/perf issues. cache/cache.go:2371
2024-04-01T12:43:56-07:00 WARN [AUTH:2:CA] Cache was forced to load session app_session/3a755c72d35cfd750e4bf012fcd5ffe92f808b7fabc39e3f8eb13bbed19fdefb from upstream. Frequent occurrence may indicate sync/perf issues. cache/cache.go:2371
2024-04-01T12:43:56-07:00 WARN [AUTH:2:CA] Cache was forced to load session app_session/3a755c72d35cfd750e4bf012fcd5ffe92f808b7fabc39e3f8eb13bbed19fdefb from upstream. Frequent occurrence may indicate sync/perf issues. cache/cache.go:2371

It seems to me like this fallback would be more performant than setting up a resource watcher, but I'm concerned about log spam and the sync issue above. wdyt?

Edit: Actually we were seeing these logs in v15 as well since WaitForAppSession calls GetAppSession first and immediately falls back for proxy cache to auth cache to auth backend. Maybe we should just demote the Warn to a Debug.

@fspmarshall
Copy link
Copy Markdown
Contributor

@Joerger The fallback logic was originally introduced to mitigate a bug, however I'm not opposed to having the fallback logic graduate to being 'expected behavior'. And yeah, I think in that case it's worth downgrading that log line to debug.

@Joerger Joerger force-pushed the joerger/remove-web-session-wait-logic branch from 40736c1 to 5ee416b Compare April 17, 2024 18:46
@Joerger Joerger enabled auto-merge April 17, 2024 18:46
@Joerger Joerger added this pull request to the merge queue Apr 17, 2024
Merged via the queue into master with commit 9479f7f Apr 17, 2024
@Joerger Joerger deleted the joerger/remove-web-session-wait-logic branch April 17, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

database-access Database access related issues and PRs machine-id size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants