Cleanup YT aggregation table #391

annamel · 2017-01-30T15:34:23Z

Periodic cleanup of YT aggregation table in order to decrease its size and thus the time of ttl_cleanup_sched time (Depends on PR366)

nobodyisme · 2017-02-18T13:54:06Z

src/cocaine-app/planner/__init__.py

            yt_attempts = self.params.get('ttl_cleanup', {}).get('yt_attempts', 3)
            yt_delay = self.params.get('ttl_cleanup', {}).get('yt_delay', 10)

-            from yt_worker import YqlWrapper


Импорт YqlWrapper специально был перенесён в метод из-за проблем зависимостей пакетов yql и yt. Нам сейчас нужно это так и оставить, потому что мы не хотим слепо обновляться до последних версий этих библиотек, это может привести к проблемам в других компоненах, которые пользуются библиотеками yql.

nobodyisme · 2017-02-18T13:56:17Z

src/cocaine-app/yt_worker.py

        WHERE  sum_expired_size >= {trigger};
    """

+    REPLACE_QUERY = """


Это запрос нужно отформатировать таким же образом, которым отформатирован AGGREGATE_QUERY выше.

А почему не таким же как PREAGGREGATE_QUERY?

Потому что PREAGGREGATE_QUERY отформатирован плохо, он нарушает структуру нативного кода.

nobodyisme · 2017-02-18T13:59:13Z

src/cocaine-app/planner/__init__.py

        self.ttl_cleanup_timer = periodic_timer(
            seconds=self.params.get('ttl_cleanup', {}).get('ttl_cleanup_period', 60 * 15))
+        self.ttl_cleanup_yt_timer = periodic_timer(
+            seconds=self.params.get('ttl_cleanup', {}).get('yt_cleanup_period', 60 * 60 * 24 * 10)


Вынеси, пожалуйста, yt_cleanup_period в examples/mastermind.conf, и оставь комментарий про 60 * 60 * 24 * 10.

nobodyisme · 2017-02-18T14:01:40Z

src/cocaine-app/planner/__init__.py

+                self._cleanup_aggregation_yt()
+
+        except LockFailedError:
+            logger.info('TTl cleanup planner or ttl yt cleaner is already running')


Видимо, сообщение в лог внутри _ttl_cleanup_planner при LockFailedError теперь должно поменяться.

fixed in 85d74e4

nobodyisme · 2017-02-18T14:03:37Z

src/cocaine-app/planner/__init__.py

+        except:
+            logger.exception('Failed to clean YT aggregation table')
+        finally:
+            logger.info('TTL YT cleaner finished')


Почему всё время скачет регистр?
https://github.com/yandex/mastermind/pull/391/files#diff-68b1944151933048b6bb46ecf5bb688fR263
https://github.com/yandex/mastermind/pull/391/files#diff-68b1944151933048b6bb46ecf5bb688fR269

Остановись, пожалуйста, на одном варианте.

Вроде fixed

nobodyisme · 2017-02-18T15:10:31Z

src/cocaine-app/yt_worker.py

+        # But in the beginning we should get update from aggregation table
+        # (i.e. values that were recorded during our processing and thus that are absent in tmp_table)
+        replace_query = self.REPLACE_QUERY.format(
+            starting_time=time.time(), tmp_table=tmp_table_name, base_table=aggregate_table)


Почему starting_time начинается от текущего момента, хотя выборка производилась раньше (возможно, сильно раньше), когда выполнялся запрос https://github.com/yandex/mastermind/pull/391/files#diff-878f10083915ff8adfd73d2409039842R243

Почему в tmp-таблицу добавляются записи, которые появились раньше starting_time, хотя в комментарии выше написано, что должны добавляться новые?
https://github.com/yandex/mastermind/pull/391/files#diff-878f10083915ff8adfd73d2409039842R82
Разъясни, пожалуйста, эту часть логики.

Была ошибка. Спасибо что обратил внимание. Починила в 85d74e4

Есть ли тесты на этот кейс? Если тесты есть, почему по ним не было видно этой проблемы? Если нет, то будут ли?

Ошибка, которую ты заметил, относится к ошибкам тестирования. Поясню. Поскольку в таблицу нет интенсивных записей (а мне не удавалось руками "поймать" момент, когда ф-ия колбасит данные и в параллель записать в таблицу; автоматизировать же этот код было лень), я на этапе каких-то последних тестов изменила знак неравенства с целью проверить, что "псевдо-новенькие" записи нормально перельются во временную таблицу. На этом "ручном" тесте я в частности отловила момент с несоответствием целочисленных типов. Но неравенство обратно поправить забыла. Я к тому, что не то чтобы этот код никогда не работал. Я его проверяла руками и смотрела таблички.

Автотестов на этот кейз нет и не представляется возможным. Да, мы можем сказать какие записи должны появиться в таблице в ходе работы теста по "основному" сценарию ttl_cleanup. Да, мы можем сказать, какие из этих записей точно должны удалиться в ходе yt_cleanup. Но поскольку и ttl_cleanup и yt_cleanup запускаются по своему расписанию и мы не контролируем их запуск, а в ходе теста мы уходим на относительно долгие операции (итерация по группам и проверка содержимого), то гарантированно проверить то, что запись была и запись удалилась, представляется маловероятным. Можно, конечно, в отдельном потоке, а лучше в отдельном worker-е, молотить по базе и смотреть что вот записи возникли и вот записи удалились. Но запрос на получение записей и анализ - тоже какое-то время. Время же жизни записи в таблице мы не можем контролировать. То есть даже в случае отдельного воркера тесты будут "вероятностные". Нормальным решением стало бы контролируемое время запуска yt_cleanup - тогда мы могли бы начиная с какой-то засечки ожидать исчезновения записей из таблицы, но такой вариант невозможен в силу текущей архитектуры мастермайнда

Я правильно понимаю, что в случае бага мы бы получили дублирующиеся записи в агрегированной таблице? Почему бы не написать тесты на то, что после чистки таблицы в новой копии у нас нет дублей?

Тест добавлен. К сожалению, он наслоился на PR6. https://github.yandex-team.ru/indigo/mastermind-qa/pull/6. Это последний патч в стопке, и да он зависит от тех изменений

nobodyisme · 2017-02-18T15:15:22Z

src/cocaine-app/planner/__init__.py

+        for couple_data in couples_data:
+            c = couple_data['couple']
+
+            if c == "None":


Такого быть не должно, и обрабатывать это не имеет смысла. Откуда взялась такая проверка?

Проверка взялась от факта наличия такой записи. Откуда-то в монгу такая капла просочилось. Не знаю, есть ли такое в проде - в тестинге возникло. И продолжает возникать

Покажи, пожалуйста, как выглядят эти записи в тестинге. Я таких не нашёл, ни в твоей монге, ни в тестовой. Вот, например, в твоей:
https://paste.yandex-team.ru/207942

[Wed Feb 22 12:15:10 2017] [ERROR] app/mastermind2.26: [mm.planner] [11636] Damaged couple? {u'couple': u'None', u'_id': ObjectId('586146b4acb2e49fc62bc61f'), u'cleanup_ts': 1485535200}

Эта запись в контейнере. sudo lxc-attach -n mastermind.aniam.lxc

Тогда тебе нужно понять, откуда такая запись появляется. Это проблема не в тестинге, а на твоём тестовом стенде. Если в тестинге такого нет, похоже, что проблема в твоём коде. Таких записей в монге появляться не должно.

nobodyisme · 2017-02-18T15:16:59Z

src/cocaine-app/planner/__init__.py

+
+            group_id = int(c.split(":")[0])
+
+            if group_id not in storage.groups:


Это условие не обязательно должно выполняться. couple_id в общем случае не обязан быть связан с номерами групп, поэтому проверять тут это не стоит.

nobodyisme · 2017-02-18T15:18:05Z

src/cocaine-app/planner/__init__.py

+                logger.error("Not valid group is within collection {}".format(group_id))
+                continue
+            group = storage.groups[group_id]
+            if not group.couple:


Couple можно попробовать получить напрямую из storage.couples, используя поле couple записи в монге. Делать поиск через группу не нужно.

Не вижу нужных изменения в коммите 85d74e4

Моя недоработка. Сначала сделала как ты сказал, а потом откатила их. Дело в том, что мы все равно нуждаемся в group_id для результата. А если мы уже и так имеем группу, то ведь недурно проверить, что она все еще coupled. И там я уже не вижу принципиальной разницы - искать в storage.couples или в storage.groups. Может статься, что в storage.groups даже быстрее

С точки зрения здравого смысла ты получаешь из коллекции каплы, а значит и надо интерпретировать их как каплы. Группу в самом конце, где тебе нужно, ты можешь спокойно получить из капла, а танцы с бубном вокруг айдишника группы тут совершенно лишние. Тем более, получая капл, ты избавляешь себя от необходимости проверки, что у группы есть капл, — получая группу из капла ты автоматически получаешь и такую гарантию.

nobodyisme · 2017-02-18T15:20:07Z

src/cocaine-app/planner/__init__.py

+
+            ns_settings = self.namespaces_settings.get(group.couple.namespace.id)
+            if ns_settings and not ns_settings.attributes.ttl.enable:
+                logger.debug("Skipping group {} cause ns '{}' no ttl".format(group_id, group.couple.namespace.id))


cause -> because
no ttl -> no ttl enabled, или даже лучше полностью написать, что because ttl is not enabled for ns {}

Обсудили в телеграмме, комментарий признан не критичным

nobodyisme · 2017-02-23T14:02:48Z

src/cocaine-app/planner/__init__.py

+        for couple_data in couples_data:
+            c = couple_data['couple']
+
+            if c == "None":


Покажи, пожалуйста, как выглядят эти записи в тестинге. Я таких не нашёл, ни в твоей монге, ни в тестовой. Вот, например, в твоей:
https://paste.yandex-team.ru/207942

nobodyisme · 2017-02-23T14:06:01Z

src/cocaine-app/yt_worker.py

+                    logger.error("couple {} not in couples_hist {}".format(couple_id, couples_hist))
+                    continue
+
+                if couples_hist[couple_id] < expiration:


Есть ли тесты на этот кейс? Если тесты есть, почему по ним не было видно этой проблемы? Если нет, то будут ли?

Я попросил тут оставить комментарий про значение None у couples_hist[couple_id], комментария в коммите не вижу.

nobodyisme · 2017-02-23T14:12:37Z

src/cocaine-app/yt_worker.py

+                converting_func = lambda r, i: "CAST({} AS {})".format(r, table.columns[i][1]) \
+                    if table.columns[i][1] != "String" else ("'" + str(r) + "'")
+
+                ins_query += "(" + ", ".join(converting_func(r, row.index(r)) for r in row) + "),"


Пока не придём, пулл-реквест принят не будет. Я не могу согласиться с такой реализаций этого метода:

Запрос приходится собирать по кускам по всему коду метода вместо того, чтобы увидеть его сразу;

Код предполагает, что специального форматирования потребует только тип "String", об остальных типах (какие они бывают, какие поддерживаются, и т.д.) из этого кода непонятно совсем;

Не видно, с какими данными из таблицы работает код.

nobodyisme · 2017-02-23T14:13:16Z

src/cocaine-app/yt_worker.py

+        # But in the beginning we should get update from aggregation table
+        # (i.e. values that were recorded during our processing and thus that are absent in tmp_table)
+        replace_query = self.REPLACE_QUERY.format(
+            starting_time=time.time(), tmp_table=tmp_table_name, base_table=aggregate_table)


Есть ли тесты на этот кейс? Если тесты есть, почему по ним не было видно этой проблемы? Если нет, то будут ли?

nobodyisme · 2017-02-23T14:20:06Z

src/cocaine-app/planner/__init__.py

+                logger.error("Not valid group is within collection {}".format(group_id))
+                continue
+            group = storage.groups[group_id]
+            if not group.couple:


Не вижу нужных изменения в коммите 85d74e4

By old records we mean records with expiration_date < last ttl_cleanup_run. That is records that have to be removed by ttl_cleanup

nobodyisme · 2017-03-12T20:22:58Z

src/cocaine-app/yt_worker.py

+        r = self.send_request(query)
+
+        try:
+            table = next(x for x in r.results)


Кажется, можно было бы просто написать next(r.results)?

вроде как да. r.results объявлен как self.result = method(
full_url,
timeout=self.timeout,
headers=headers
)
https://a.yandex-team.ru/api/tree/blob/trunk/arcadia/kikimr/yql/library/python/yql/client/request.py/?rev=2606973
Но при этом попытка получить next приводила к exception-у (если я правильно помню что-то типа (object is not an iterator). Если это важно, могу воспроизвести и разобраться, почему все-таки был exception.

Тогда next(iter(r.results)) должен помочь.

nobodyisme · 2017-03-12T20:23:35Z

src/cocaine-app/yt_worker.py

+        try:
+            table = next(x for x in r.results)
+        except:
+            logger.exception("Incorrect results of query {}".format(query))


В случае исключения, кажется, продолжать нельзя, нужен return.

nobodyisme · 2017-03-12T20:28:28Z

src/cocaine-app/yt_worker.py

+            value_type = table.columns[value_idx][1]
+            if value_type == "String":
+                return "'{}'".format(value)  # add extra '
+            if value_type == "Uint64" or value_type == "Int64" or value_type == "Int32" or value_type == "Uint32":


if value_type in ("Uint64", "Int64", "Int32", "Uint32"):

nobodyisme · 2017-03-12T20:28:55Z

src/cocaine-app/yt_worker.py

+                return "'{}'".format(value)  # add extra '
+            if value_type == "Uint64" or value_type == "Int64" or value_type == "Int32" or value_type == "Uint32":
+                return "CAST({} AS {})".format(value, value_type)
+            raise ValueError("Unsupported type in aggregation table")


Сюда можно было бы добавить этот самый неподдерживаемый тип и даже название колонки.

nobodyisme · 2017-03-12T20:31:22Z

src/cocaine-app/yt_worker.py

+        ins_query = "INSERT INTO [{tmp_table_name}] ({values_names}) VALUES {values};".format(
+            tmp_table_name=tmp_table_name,
+            values_names=", ".join(cln[0] for cln in table.columns),
+            values=", ".join("({})".format(", ".join(convertion(r, row.index(r)) for r in row)) for row in values_to_insert))


convertion(r, row.index(r)) for r in row)) лучше заменить на convertion(r, i) for i, r in enumerate(row). Операция index() тут не нужна, не говоря уже о том, что она дорогая.

Спасибо!

nobodyisme · 2017-03-12T20:39:54Z

src/cocaine-app/planner/__init__.py

+        for couple_data in couples_data:
+            c = couple_data['couple']
+
+            if c == "None":


Тогда тебе нужно понять, откуда такая запись появляется. Это проблема не в тестинге, а на твоём тестовом стенде. Если в тестинге такого нет, похоже, что проблема в твоём коде. Таких записей в монге появляться не должно.

nobodyisme · 2017-03-12T20:47:09Z

src/cocaine-app/planner/__init__.py

+                logger.error("Not valid group is within collection {}".format(group_id))
+                continue
+            group = storage.groups[group_id]
+            if not group.couple:


С точки зрения здравого смысла ты получаешь из коллекции каплы, а значит и надо интерпретировать их как каплы. Группу в самом конце, где тебе нужно, ты можешь спокойно получить из капла, а танцы с бубном вокруг айдишника группы тут совершенно лишние. Тем более, получая капл, ты избавляешь себя от необходимости проверки, что у группы есть капл, — получая группу из капла ты автоматически получаешь и такую гарантию.

nobodyisme · 2017-03-12T20:59:08Z

src/cocaine-app/yt_worker.py

+                    logger.error("couple {} not in couples_hist {}".format(couple_id, couples_hist))
+                    continue
+
+                if couples_hist[couple_id] < expiration:


Про тесты: баг, который я заметил, приводил бы к тому, что мы теряли все полезные данные из агрегированной таблицы. Так что меня абсолютно не устраивает, что тестов на этот кейс нет. Этот кейс обязательно надо покрыть.

annamel force-pushed the ttl_sched_yt_cleanup branch from 4200bc8 to 4d334cf Compare February 10, 2017 15:03

nobodyisme requested changes Feb 18, 2017

View reviewed changes

nobodyisme requested changes Feb 23, 2017

View reviewed changes

annamel force-pushed the ttl_sched_yt_cleanup branch from 63704a1 to 96fc2fd Compare March 2, 2017 13:22

Anna Melekhova added 4 commits March 2, 2017 16:35

Cleanup YT aggregation table from old records

49c1a23

By old records we mean records with expiration_date < last ttl_cleanup_run. That is records that have to be removed by ttl_cleanup

Fixes for review. To be merged

38ec8da

Fixup to prev. Add comment

b10a375

Fixup of insert request

96fc2fd

nobodyisme reviewed Mar 12, 2017

View reviewed changes

Anna Melekhova added 2 commits March 14, 2017 17:05

Tmp changes

664a820

Fixes

2fd91c3


		group_id = int(c.split(":")[0])

		if group_id not in storage.groups:

Cleanup YT aggregation table #391

Are you sure you want to change the base?

Cleanup YT aggregation table #391

Uh oh!

Conversation

annamel commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nobodyisme Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annamel Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nobodyisme Feb 21, 2017 •

edited

Loading

annamel Apr 10, 2017 •

edited

Loading