fix: index tag info into elasticsearch immediately after ui change #883
Conversation
Signed-off-by: tianru zhou <tianru.zhou@databricks.com>
tianruzhou-db
requested review from
allisonsuarez,
dikshathakur3119,
dorianj,
feng-tao and
ttannis
as code owners
| @@ -377,6 +377,75 @@ def get_tags() -> Response: | |||
| return make_response(payload, HTTPStatus.INTERNAL_SERVER_ERROR) | |||
|
|
|||
|
|
|||
| def _update_metadata_service(table_key: str, method: str, tag: str) -> int: | |||
There was a problem hiding this comment.
is the method only used in tag update?
There was a problem hiding this comment.
maybe called def _update_metadata_tag ?
There was a problem hiding this comment.
yes, it's only used in tag update. will make the change
| return status_code | ||
|
|
||
|
|
||
| def _update_search_service(table_key: str, method: str, tag: str) -> int: |
There was a problem hiding this comment.
maybe called def _update_search_tag ?
| return status_code | ||
|
|
||
|
|
||
| def _update_search_service(table_key: str, method: str, tag: str) -> int: |
There was a problem hiding this comment.
would be good to add a docstring here to discuss the flow
|
|
||
| def _update_search_service(table_key: str, method: str, tag: str) -> int: | ||
| searchservice_base = app.config['SEARCHSERVICE_BASE'] | ||
| searchservice_get_table_url = f'{searchservice_base}/search_table' |
There was a problem hiding this comment.
could you add a todo which we should update for dashboard tag in the future as well (current search update doesn't support dashboard)
| database = table_key[:table_key.find('://')] | ||
| schema = table_key[table_key.find('.') + 1:table_key.rfind('/')] | ||
| table = table_key[table_key.rfind('/') + 1:] | ||
| cluster = table_key[table_key.find('://') + 3:table_key.find('.')] |
There was a problem hiding this comment.
@ttannis @allisonsuarez does FE have any util to retrieve the different field from a table key?
This looks a bit cryptic, could be rewritten as:
import re
m = re.match(r'([^./]+)://([^./]+)\.([^./]+)\/([^./]+)', table_key)
db=m.group(1),
cluster=m.group(2),
schema=m.group(3),
table =m.group(4)
There was a problem hiding this comment.
in amundsen_application.api.utils.metadata_utils we have TableUri which can be used to parse
|
|
||
| raw_data_map = json.loads(get_table_response.text) | ||
| # key is unique, thus (database, cluster, schema, table) should uniquely identify the table | ||
| table = raw_data_map['results'][0] |
There was a problem hiding this comment.
does the result have only single element?
There was a problem hiding this comment.
Yes, if everything works correctly, we can add checks here
|
|
||
| old_tags_list = table['tags'] | ||
| new_tags_list = [item for item in old_tags_list if item['tag_name'] != tag] | ||
| if method != 'DELETE': |
There was a problem hiding this comment.
if we delete a tag, will it update the ES index as well ?
There was a problem hiding this comment.
Yes, the logic is exactly the same as put
| @@ -43,15 +44,17 @@ def request_metadata(*, # type: ignore | |||
| client=app.config['METADATASERVICE_REQUEST_CLIENT'], | |||
| headers=headers, | |||
| timeout_sec=timeout_sec, | |||
| data=data) | |||
| data=data, | |||
There was a problem hiding this comment.
any reason we can't use data but need another json input param?
There was a problem hiding this comment.
For json input, we can pass dict directly. I try to use json.dumps(dict) combined with data, but seems that it can't be recognized by search service.
|
CI fails as well |
Signed-off-by: tianru zhou <tianru.zhou@databricks.com>
Signed-off-by: tianru zhou <tianru.zhou@databricks.com>
Signed-off-by: tianru zhou <tianru.zhou@databricks.com>
Signed-off-by: tianru zhou tianru.zhou@databricks.com
Summary of Changes
After add / delete tags for tables in UI, update corresponding tags info in elasticsearch synchronously.
Related SearchService PR: amundsen-io/amundsensearchlibrary#172
Tests
Documentation
CheckList
Make sure you have checked all steps below to ensure a timely review.