Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
dimas-b commented on PR #4046: URL: https://github.com/apache/polaris/pull/4046#issuecomment-4136134424 +1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
MonkeyCanCode merged PR #4046: URL: https://github.com/apache/polaris/pull/4046 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
MonkeyCanCode commented on PR #4046: URL: https://github.com/apache/polaris/pull/4046#issuecomment-4127897299 Thanks for the quick review @flyrain . Any other concerns before I merge this @jbonofre @dimas-b ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
MonkeyCanCode commented on code in PR #4046:
URL: https://github.com/apache/polaris/pull/4046#discussion_r2985190936
##
client/python/apache_polaris/cli/command/setup.py:
##
@@ -1196,6 +1197,22 @@ def _create_namespaces(
all_namespaces_to_create.add(parent_ns_name)
sorted_namespaces = sorted(list(all_namespaces_to_create))
for ns_name in sorted_namespaces:
+parts = ns_name.split(".")
+if len(parts) > 1:
+parent_ns = ".".join(parts[:-1])
+if parent_ns in existing_namespaces and parent_ns not in
listed_parents:
Review Comment:
Removed the unnecessary pre-fetch and keep only the root there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
MonkeyCanCode commented on code in PR #4046:
URL: https://github.com/apache/polaris/pull/4046#discussion_r2985153209
##
client/python/apache_polaris/cli/command/setup.py:
##
@@ -1225,6 +1243,12 @@ def _create_namespaces(
logger.info(
f"Namespace '{ns_name}' created successfully in
catalog '{catalog_name}'."
)
+existing_namespaces.add(ns_name)
+except ConflictException:
+logger.info(
+f"Namespace '{ns_name}' already exists in catalog
'{catalog_name}'."
+)
+existing_namespaces.add(ns)
Review Comment:
Nice catch. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
dimas-b commented on code in PR #4046:
URL: https://github.com/apache/polaris/pull/4046#discussion_r2982529848
##
client/python/apache_polaris/cli/command/setup.py:
##
@@ -1225,6 +1243,12 @@ def _create_namespaces(
logger.info(
f"Namespace '{ns_name}' created successfully in
catalog '{catalog_name}'."
)
+existing_namespaces.add(ns_name)
+except ConflictException:
+logger.info(
+f"Namespace '{ns_name}' already exists in catalog
'{catalog_name}'."
+)
+existing_namespaces.add(ns)
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
dimas-b commented on code in PR #4046:
URL: https://github.com/apache/polaris/pull/4046#discussion_r2982517174
##
client/python/apache_polaris/cli/command/setup.py:
##
@@ -1196,6 +1197,22 @@ def _create_namespaces(
all_namespaces_to_create.add(parent_ns_name)
sorted_namespaces = sorted(list(all_namespaces_to_create))
for ns_name in sorted_namespaces:
+parts = ns_name.split(".")
+if len(parts) > 1:
+parent_ns = ".".join(parts[:-1])
+if parent_ns in existing_namespaces and parent_ns not in
listed_parents:
Review Comment:
I guess this logic could be simplified WRT lines 1164-1166. We could start
from root (empty) NS, which always exists, and populate `existing_namespaces`
only in this loop. The `existing_namespaces` does not have to be checked, it
will only accumulate results. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] CLI: Skip nested ns if existed during setup [polaris]
jbonofre commented on code in PR #4046:
URL: https://github.com/apache/polaris/pull/4046#discussion_r2979098465
##
client/python/apache_polaris/cli/command/setup.py:
##
@@ -1225,6 +1243,12 @@ def _create_namespaces(
logger.info(
f"Namespace '{ns_name}' created successfully in
catalog '{catalog_name}'."
)
+existing_namespaces.add(ns_name)
+except ConflictException:
+logger.info(
+f"Namespace '{ns_name}' already exists in catalog
'{catalog_name}'."
+)
+existing_namespaces.add(ns)
Review Comment:
I think it should be `ns_name` here as `ns` is actually the loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
