Skip to content

Add missing ability to configure QueryClientSettings #512

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 3 commits into from
Oct 28, 2024

Conversation

vgvoleg
Copy link
Collaborator

@vgvoleg vgvoleg commented Oct 23, 2024

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Other information

@vgvoleg vgvoleg force-pushed the query_client_settings branch from 41caeac to 8c43592 Compare October 23, 2024 14:16
Copy link

github-actions bot commented Oct 23, 2024

🌋 Here are results of SLO test for Python SDK over Query Service:

Grafana Dashboard

SLO-sync-python-query

Copy link

github-actions bot commented Oct 23, 2024

🌋 Here are results of SLO test for Python SDK over Table Service:

Grafana Dashboard

SLO-sync-python-table

@vgvoleg vgvoleg force-pushed the query_client_settings branch 2 times, most recently from 8c3e524 to f0a84f6 Compare October 24, 2024 10:12
@vgvoleg vgvoleg force-pushed the query_client_settings branch from f0a84f6 to e757eef Compare October 24, 2024 10:15
@vgvoleg vgvoleg requested a review from rekby October 24, 2024 13:06
return settings


test_td = timedelta(microseconds=-100)
Copy link
Member

Choose a reason for hiding this comment

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

why do you need 100 microseconds?
can you use one second or one millisecond?


test_td = timedelta(microseconds=-100)
test_now = datetime.utcnow()
test_today = test_now.date()
Copy link
Member

Choose a reason for hiding this comment

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

what about use fixed date/time instead of now?



params = pytest.mark.parametrize(
"value,ydb_type,casted_result,uncasted_type",
Copy link
Member

Choose a reason for hiding this comment

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

casted_result and uncasted_type are difficult to underdstand.

what about: raw result and native_result?
and always compare values

self,
driver: common_utils.SupportedDriverType,
size: int = 100,
query_client_settings: Optional[QueryClientSettings] = None,
Copy link
Member

Choose a reason for hiding this comment

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

what about explicit named paremeter for the settings?

self,
driver: common_utils.SupportedDriverType,
size: int = 100,
query_client_settings: Optional[QueryClientSettings] = None,
Copy link
Member

Choose a reason for hiding this comment

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

and here (named parameter)

@vgvoleg vgvoleg force-pushed the query_client_settings branch from db6de2e to f908698 Compare October 25, 2024 15:18
@vgvoleg vgvoleg force-pushed the query_client_settings branch from f908698 to 54bf667 Compare October 25, 2024 15:27
@vgvoleg vgvoleg merged commit 8674169 into main Oct 28, 2024
11 checks passed
@vgvoleg vgvoleg deleted the query_client_settings branch October 28, 2024 07:04
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.

2 participants