Skip to content

Add workload routing connection property#508

Merged
sitingren merged 10 commits into
vertica:masterfrom
DMickens:add-workload-routing-connection-property
May 2, 2023
Merged

Add workload routing connection property#508
sitingren merged 10 commits into
vertica:masterfrom
DMickens:add-workload-routing-connection-property

Conversation

@DMickens
Copy link
Copy Markdown
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
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
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
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.

Comment thread vertica_python/vertica/connection.py Outdated
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
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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

Siting Ren added 2 commits May 2, 2023 09:28
@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
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