Skip to content
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

Add workload routing connection property #508

Merged

Conversation

DMickens
Copy link
Contributor

@DMickens DMickens commented Apr 27, 2023

Adding workload routing connection property which will be available in Vertica 23.3 release.

@DMickens DMickens requested a review from sitingren April 27, 2023 23:55
@@ -95,6 +95,7 @@ def __init__(self, user, database, session_label, os_user_name, autocommit,
b'binary_data_protocol': '1' if binary_transfer else '0', # Defaults to text format '0'
b'protocol_features': '{"request_complex_types":' + request_complex_types + '}',
b'protocol_compat': 'VER',
b'workload': workload,
Copy link
Member

@sitingren sitingren May 2, 2023

Choose a reason for hiding this comment

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

Does this need protocol version increase?
Your test calls self.require_protocol_at_least(3 << 16 | 15), but the request version in vertica_python/__init__.py is still 3.14.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually since we are just querying the dc client server messages table it won't require a protocol version increase. I'll remove this

Copy link
Member

@sitingren sitingren May 2, 2023

Choose a reason for hiding this comment

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

Please don't remove require_protocol_at_least(). Querying the dc client server messages table need to have server greater than a certain version, so that dc_client_server_messages table is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also requires "SHOW WORKLOAD" which I've implemented on the server in version 23.3. This test can't pass until we have a Vertica image available for 23.3 which is not yet released so it will be a while.

@@ -117,7 +118,7 @@ def parse_dsn(dsn):
continue
elif key in ('connection_load_balance', 'use_prepared_statements',
'disable_copy_local', 'ssl', 'autocommit',
'binary_transfer', 'request_complex_types'):
'binary_transfer', 'request_complex_types', 'workload'):
Copy link
Member

@sitingren sitingren May 2, 2023

Choose a reason for hiding this comment

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

This is for boolean type parameters. Parameter 'workload' accepts and passes a string to the server. So no need to add it at here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks


def test_workload_default(self):
with self._connect() as conn:
self.require_protocol_at_least(3 << 16 | 15)
Copy link
Member

@sitingren sitingren May 2, 2023

Choose a reason for hiding this comment

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

This require_protocol_at_least() should be moved to the top of the test function.

query = """SELECT contents FROM dc_client_server_messages
WHERE session_id = current_session() " +
AND message_type = '^+' " +
AND contents LIKE '%workload%'");"""
Copy link
Member

Choose a reason for hiding this comment

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

Syntax error in this multiline string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, came from copying and pasting a concatenated string

AND message_type = '^+' " +
AND contents LIKE '%workload%'");"""
res = self._query_and_fetchone(query)
self.assertEqual(res[0], 'python_test_workload')
Copy link
Member

Choose a reason for hiding this comment

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

The result should be equal to 'workload: python_test_workload', rather than 'python_test_workload'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are correct

@DMickens DMickens force-pushed the add-workload-routing-connection-property branch from bd855fe to e621fb6 Compare May 2, 2023 12:48
@sitingren sitingren merged commit 5166f2f into vertica:master May 2, 2023
6 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.

None yet

2 participants