Return False when failed to migrate table

[JCZuurmond]

Return False when failed to migrate.

[JCZuurmond]

@FastLee : See the following code that you introduced in PR:

ucx/src/databricks/labs/ucx/hive_metastore/table_migration_status.py

Lines 121 to 124 in 2209174

	except NotFound:
	# If the source table doesn't exist, it will not be shown as migrated
	logger.warning(f"failed-to-migrate: {schema}.{table} set as a source does no longer exist")
	return True

The comment contradicts the return statement, should it be return False instead?

[JCZuurmond]

@nfx and @FastLee : In migrate grants we have:

ucx/src/databricks/labs/ucx/hive_metastore/grants.py

Lines 740 to 754 in 2209174

	def apply(self, src: Table, uc_table_key: str) -> bool:
	for grant in self._match_grants(src):
	acl_migrate_sql = grant.uc_grant_sql(src.kind, uc_table_key)
	if acl_migrate_sql is None:
	logger.warning(
	f"failed-to-migrate: Hive metastore grant '{grant.action_type}' cannot be mapped to UC grant for "
	f"{src.kind} '{uc_table_key}'. Skipping."
	)
	continue
	logger.debug(f"Migrating acls on {uc_table_key} using SQL query: {acl_migrate_sql}")
	try:
	self._sql_backend.execute(acl_migrate_sql)
	except DatabricksError as e:
	logger.warning(f"failed-to-migrate: Failed to migrate ACL for {src.key} to {uc_table_key}: {e}")
	return True

I understand we skip some unmappable grants (e.g. READ_METADATA):

ucx/src/databricks/labs/ucx/hive_metastore/grants.py

Lines 742 to 748 in 2209174

	acl_migrate_sql = grant.uc_grant_sql(src.kind, uc_table_key)
	if acl_migrate_sql is None:
	logger.warning(
	f"failed-to-migrate: Hive metastore grant '{grant.action_type}' cannot be mapped to UC grant for "
	f"{src.kind} '{uc_table_key}'. Skipping."
	)
	continue

However, should we consider the migration successful when a mappable grant can not be executed?

ucx/src/databricks/labs/ucx/hive_metastore/grants.py

Lines 750 to 753 in 2209174

	try:
	self._sql_backend.execute(acl_migrate_sql)
	except DatabricksError as e:
	logger.warning(f"failed-to-migrate: Failed to migrate ACL for {src.key} to {uc_table_key}: {e}")

I would argue the migration is unsuccessful, thus return False

[github-actions]

✅ 53/53 passed, 1 flaky, 4 skipped, 1h44m7s total

Flaky tests:

• 🤪 test_migration_job_ext_hms[regular] (6m16.414s)

_{Running from acceptance #6854}

[FastLee]

@nfx and @FastLee : In migrate grants we have:

ucx/src/databricks/labs/ucx/hive_metastore/grants.py

Lines 740 to 754 in 2209174

	def apply(self, src: Table, uc_table_key: str) -> bool:
	for grant in self._match_grants(src):
	acl_migrate_sql = grant.uc_grant_sql(src.kind, uc_table_key)
	if acl_migrate_sql is None:
	logger.warning(
	f"failed-to-migrate: Hive metastore grant '{grant.action_type}' cannot be mapped to UC grant for "
	f"{src.kind} '{uc_table_key}'. Skipping."
	)
	continue
	logger.debug(f"Migrating acls on {uc_table_key} using SQL query: {acl_migrate_sql}")
	try:
	self._sql_backend.execute(acl_migrate_sql)
	except DatabricksError as e:
	logger.warning(f"failed-to-migrate: Failed to migrate ACL for {src.key} to {uc_table_key}: {e}")
	return True

I understand we skip some unmappable grants (e.g. READ_METADATA):

ucx/src/databricks/labs/ucx/hive_metastore/grants.py

Lines 742 to 748 in 2209174

	acl_migrate_sql = grant.uc_grant_sql(src.kind, uc_table_key)
	if acl_migrate_sql is None:
	logger.warning(
	f"failed-to-migrate: Hive metastore grant '{grant.action_type}' cannot be mapped to UC grant for "
	f"{src.kind} '{uc_table_key}'. Skipping."
	)
	continue

However, should we consider the migration successful when a mappable grant can not be executed?

ucx/src/databricks/labs/ucx/hive_metastore/grants.py

Lines 750 to 753 in 2209174

	try:
	self._sql_backend.execute(acl_migrate_sql)
	except DatabricksError as e:
	logger.warning(f"failed-to-migrate: Failed to migrate ACL for {src.key} to {uc_table_key}: {e}")

I would argue the migration is unsuccessful, thus return False

I would argue we have to keep it as True. We may want to consider ACL migration status.

[FastLee]

LGTM

[FastLee]

@FastLee : See the following code that you introduced in PR:

ucx/src/databricks/labs/ucx/hive_metastore/table_migration_status.py

Lines 121 to 124 in 2209174

	except NotFound:
	# If the source table doesn't exist, it will not be shown as migrated
	logger.warning(f"failed-to-migrate: {schema}.{table} set as a source does no longer exist")
	return True

The comment contradicts the return statement, should it be return False instead?

This is a case where the old table is removed. The migration status should be successful. Otherwise we will not be able to migrate views.

[JCZuurmond]

This is a case where the old table is removed. The migration status should be successful. Otherwise we will not be able to migrate views.

Oke, then I will update the comment as that is not what is says