Skip to content

Cache CloudInfo / CloudSettings by authority #583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
May 21, 2025
Merged

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented May 20, 2025

This PR modifies CloudSettings to cache cloud information by authority (schema, host, and port) rather than by the full URL. This ensures that multiple URLs pointing to the same cluster with different paths will share the same cached CloudInfo object.

Changes Made

  1. Modified _normalize_uri method in CloudSettings class:

    • Now extracts and returns only the authority part (scheme + netloc) from URLs
    • The netloc component includes both host and port when present
  2. Updated get_cloud_info_for_cluster method:

    • Changed all references to use the normalized authority as the cache key
  3. Created comprehensive tests:

    • Tests that _normalize_uri correctly extracts just the authority part
    • Tests that URLs with the same authority (but different paths) share the same CloudInfo cache entry
    • Tests that URLs with different ports are cached separately

Example

Before this change:

  • https://cluster.kusto.windows.net/
  • https://cluster.kusto.windows.net/database1
  • https://cluster.kusto.windows.net/v1/rest

Would all be cached separately, requiring multiple network requests for the same cluster.

After this change:
All these URLs will use the same cached CloudInfo object for https://cluster.kusto.windows.net, improving efficiency and consistency.

Fixes #582.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Kusto cloud info - cache per authority Cache CloudInfo / CloudSettings by authority May 20, 2025
@Copilot Copilot AI requested a review from AsafMah May 20, 2025 10:01
Copilot finished work on behalf of AsafMah May 20, 2025 10:01
Copy link

github-actions bot commented May 20, 2025

Test Results

    6 files  ± 0      6 suites  ±0   22m 17s ⏱️ +44s
  317 tests + 3    282 ✅ + 3   35 💤 ±0  0 ❌ ±0 
1 902 runs  +18  1 692 ✅ +18  210 💤 ±0  0 ❌ ±0 

Results for commit ecaeff5. ± Comparison against base commit 7f76fb8.

♻️ This comment has been updated with latest results.

@AsafMah
Copy link
Collaborator

AsafMah commented May 20, 2025

Formatting - please run black

Copy link
Contributor Author

Copilot AI commented May 20, 2025

Formatting - please run black

I've run Black on all the modified files. The formatting has been applied in commit 940b938.

Copilot finished work on behalf of AsafMah May 20, 2025 10:08
@AsafMah
Copy link
Collaborator

AsafMah commented May 20, 2025

Note that there are existing tests in test_e2e_data.py

Make sure they work and cover the new cases

@AsafMah
Copy link
Collaborator

AsafMah commented May 20, 2025

The changelog should be for a new version, under Unreleased

@AsafMah
Copy link
Collaborator

AsafMah commented May 20, 2025

In your tests, you use unittests. We use the pytest library. Please fix

Copy link
Contributor Author

Copilot AI commented May 20, 2025

In your tests, you use unittests. We use the pytest library. Please fix

I've converted the tests in test_cloud_settings.py from unittest to pytest style in commit a3a8587. The changes include:

  1. Replacing the unittest class with standalone test functions
  2. Converting unittest setUp method to a pytest fixture with clear_cache
  3. Changing all unittest assertions (self.assertEqual, self.assertIn, self.assertIs) to pytest's plain assert statements
  4. Removing the unittest.main() call at the end of the file

The tests are now consistent with the pytest testing approach used in other parts of the repository.

Copy link
Collaborator

@AsafMah AsafMah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo all the changes in e2e

Copy link
Contributor Author

Copilot AI commented May 21, 2025

Undo all the changes in e2e

I've undone all the changes to the e2e tests by reverting test_e2e_data.py to its original state in commit ecaeff5.

@AsafMah AsafMah marked this pull request as ready for review May 21, 2025 08:00
@AsafMah AsafMah merged commit d5fcedf into master May 21, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kusto cloud info - cache per authority
3 participants