Closes #904 - Add Lakehouse Shortcuts Smart Publishing Feature#916
Closes #904 - Add Lakehouse Shortcuts Smart Publishing Feature#916bckstrm wants to merge 1 commit into
Conversation
|
@microsoft-github-policy-service agree |
bckstrm
force-pushed
the
shortcut_smart_diff
branch
from
858e832 to
9dc4339
Compare
bckstrm
force-pushed
the
shortcut_smart_diff
branch
from
9dc4339 to
e53b160
Compare
shirasassoon
left a comment
shirasassoon
left a comment
There was a problem hiding this comment.
Since there is a new feature flag, it should be documented in optional_features.md, the new flag and description should be added to the table, however, you can also add a paragraph under the selective deployment features to provide additional information.
| ENABLE_SHORTCUT_PUBLISH = "enable_shortcut_publish" | ||
| """Set to enable deploying shortcuts with the lakehouse.""" | ||
| ENABLE_SHORTCUT_SMART_DIFF = "enable_shortcut_smart_diff" | ||
| """Set to enable change comparision before deploying shortcuts""" |
| logger.info(f"Publishing Lakehouse '{self.item_obj.name}' Shortcuts") | ||
| shortcut_paths_to_unpublish = [path for path in deployed_shortcuts if path not in shortcuts_to_publish] | ||
| shortcut_paths_to_unpublish = [ | ||
| path for path in list(deployed_shortcuts.keys()) if path not in shortcuts_to_publish |
There was a problem hiding this comment.
not needed as dicts are directly iterable:
shortcut_paths_to_unpublish = [path for path in deployed_shortcuts if ...]
| ShortcutPublisher(mock_fabric_workspace, mock_item).publish_all() | ||
|
|
||
| post_calls = get_shortcut_post_calls(mock_fabric_workspace) | ||
| assert len(post_calls) == 1 |
There was a problem hiding this comment.
There's no test verifying that a brand-new shortcut (present in repo, absent from deployed) is always published even with smart diff enabled.
| yield | ||
| constants.FEATURE_FLAG.clear() | ||
| constants.FEATURE_FLAG.update(original_flags) | ||
|
|
There was a problem hiding this comment.
The fixture is created, but not used anywhere - either remove or use it instead of try/finally
| shortcuts_to_publish = {_get_shortcut_path(shortcut): shortcut for shortcut in shortcuts} | ||
|
|
||
| if shortcuts_to_publish: | ||
| logger.info(f"Publishing Lakehouse '{self.item_obj.name}' Shortcuts") |
There was a problem hiding this comment.
This logging can be misleading if all shortcuts are skipped, might make more sense to move it after the if FeatureFlag.ENALE_SHORTCUT_SMART_DIFF.value in constants.FEATURE_FLAG: block
| True when the shortcut differs from deployed state and should be published. | ||
| """ | ||
| shortcut_for_compare = replace_default_lakehouse_id(copy.deepcopy(shortcut), self.item_obj) | ||
| return not self._is_shortcut_subset_match(shortcut_for_compare, deployed_shortcut) |
There was a problem hiding this comment.
deepcopy can add more overhead than necessary when multiple shortcuts are present, might make sense to use a targeted approach.
| ] | ||
| self._unpublish_shortcuts(shortcut_paths_to_unpublish) | ||
| # Deploy and overwrite shortcuts | ||
| if FeatureFlag.ENABLE_SHORTCUT_SMART_DIFF.value in constants.FEATURE_FLAG: |
There was a problem hiding this comment.
This flag technically only works when the feature flag enable_shortcut_publish is also present in the deployment - we need to clearly document that both must be enabled for this feature to work
|
@bckstrm Thanks for the contribution! Please see my review comments and resolve the conflicts as well. Thanks |
|
@bckstrm Reminder this PR has comments for you to review, if we don't hear from you then we will close it. |
2 participants
This PR implements new feature flag
ENABLE_SHORTCUT_SMART_DIFFwhich optimizesShortcutPublisher.publish_all()in_lakehouse.pyby avoiding unnecessary republishes of shortcuts that are already deployed and whose definitions have not changed.Previously,
publish_all()republished every shortcut on each run, even when the deployed shortcut already matched the desired state. In environments with many shortcuts, this led to unnecessary API calls and longer deployment times.With this change, the workflow compares the local shortcut definition against
deployed_shortcutsand only republishes shortcuts when a change is detected.Linked Issue (REQUIRED)
#904