You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/03/25 21:21:28 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #15017: Faster default role syncing during webserver start
jedcunningham opened a new pull request #15017:
URL: https://github.com/apache/airflow/pull/15017
This makes a handful of bigger queries instead of many queries when
syncing the default Airflow roles. On my machine with 5k DAGs, this led
to a reduction of 1 second in startup time (bonus, makes tests faster
too).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#discussion_r602442775
##########
File path: airflow/www/security.py
##########
@@ -171,12 +172,26 @@ def __init__(self, appbuilder):
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
self.perms = None
+ def init_role(self, role_name, perms):
+ """
+ Initialize the role with the permissions and related view-menus.
+ :param role_name:
+ :param perms:
+ :return:
+ """
+ warnings.warn(
+ "`init_role` has been deprecated. Please use `sync_default_roles` instead.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ self.sync_default_roles([{'role': role_name, 'perms': perms}])
Review comment:
Yeah... `sync_roles` is the natural fit, but it's taken. I'll figure something out.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#discussion_r602193600
##########
File path: airflow/www/security.py
##########
@@ -467,6 +473,34 @@ def get_all_permissions(self) -> Set[Tuple[str, str]]:
.all()
)
+ def get_all_non_dag_permissionviews(self) -> Dict[Tuple[str, str], PermissionView]:
Review comment:
```suggestion
def _get_all_non_dag_permissionviews(self) -> Dict[Tuple[str, str], PermissionView]:
```
maybe?
##########
File path: airflow/www/security.py
##########
@@ -467,6 +473,34 @@ def get_all_permissions(self) -> Set[Tuple[str, str]]:
.all()
)
+ def get_all_non_dag_permissionviews(self) -> Dict[Tuple[str, str], PermissionView]:
+ """
+ Returns a dict with a key of (perm name, view menu name) and value of perm view
+ with all perm views except those that are for specific DAGs.
+ """
+ return {
+ (perm_name, viewmodel_name): viewmodel
+ for perm_name, viewmodel_name, viewmodel in (
+ self.get_session.query(self.permissionview_model)
+ .join(self.permission_model)
+ .join(self.viewmenu_model)
+ .filter(~self.viewmenu_model.name.like(f"{permissions.RESOURCE_DAG_PREFIX}%"))
+ .with_entities(
+ self.permission_model.name, self.viewmenu_model.name, self.permissionview_model
+ )
+ .all()
+ )
+ }
+
+ def get_all_roles_with_permissions(self) -> Dict[str, Role]:
Review comment:
```suggestion
def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
```
too?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #15017:
URL: https://github.com/apache/airflow/pull/15017
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#discussion_r602192975
##########
File path: airflow/www/security.py
##########
@@ -171,19 +171,25 @@ def __init__(self, appbuilder):
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
self.perms = None
- def init_role(self, role_name, perms):
Review comment:
We possibly need to keep (but deprecate) this method -- as this is part of a quasi-public API and may be in use by people outside of the Airflow codebase.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#discussion_r602438881
##########
File path: airflow/www/security.py
##########
@@ -467,6 +473,34 @@ def get_all_permissions(self) -> Set[Tuple[str, str]]:
.all()
)
+ def get_all_non_dag_permissionviews(self) -> Dict[Tuple[str, str], PermissionView]:
Review comment:
Done.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#issuecomment-808521800
@jedcunningham Could you rebase please -- master had a bug.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#discussion_r602439745
##########
File path: airflow/www/security.py
##########
@@ -171,12 +172,26 @@ def __init__(self, appbuilder):
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
self.perms = None
+ def init_role(self, role_name, perms):
+ """
+ Initialize the role with the permissions and related view-menus.
+ :param role_name:
+ :param perms:
+ :return:
+ """
+ warnings.warn(
+ "`init_role` has been deprecated. Please use `sync_default_roles` instead.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ self.sync_default_roles([{'role': role_name, 'perms': perms}])
Review comment:
Oh wait. the "default" part of the name probably doesn't make sense when called with custom role info. Time to rename it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#discussion_r602439448
##########
File path: airflow/www/security.py
##########
@@ -467,6 +473,34 @@ def get_all_permissions(self) -> Set[Tuple[str, str]]:
.all()
)
+ def get_all_non_dag_permissionviews(self) -> Dict[Tuple[str, str], PermissionView]:
+ """
+ Returns a dict with a key of (perm name, view menu name) and value of perm view
+ with all perm views except those that are for specific DAGs.
+ """
+ return {
+ (perm_name, viewmodel_name): viewmodel
+ for perm_name, viewmodel_name, viewmodel in (
+ self.get_session.query(self.permissionview_model)
+ .join(self.permission_model)
+ .join(self.viewmenu_model)
+ .filter(~self.viewmenu_model.name.like(f"{permissions.RESOURCE_DAG_PREFIX}%"))
+ .with_entities(
+ self.permission_model.name, self.viewmenu_model.name, self.permissionview_model
+ )
+ .all()
+ )
+ }
+
+ def get_all_roles_with_permissions(self) -> Dict[str, Role]:
Review comment:
Done.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#issuecomment-808815888
[The Workflow run](https://github.com/apache/airflow/actions/runs/693760960) is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#issuecomment-808395503
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15017: Faster default role syncing during webserver start
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15017:
URL: https://github.com/apache/airflow/pull/15017#discussion_r602438708
##########
File path: airflow/www/security.py
##########
@@ -171,19 +171,25 @@ def __init__(self, appbuilder):
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
self.perms = None
- def init_role(self, role_name, perms):
Review comment:
Done.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org