[AIRFLOW-4067] Telegram hook/operator to post messages to telegram channels #4891
Conversation
|
Hi @dchaplinsky , any code change (including adding new code) should come with a JIRA ticket. Only pure doc change can be exempted. |
|
@XD-DENG , just to clarify. If I want to add something to contrib, not the core, I should create a ticket in Jira first? |
|
@dchaplinsky yes, it should cover all code changes in this repository, including |
| telegram_client.call("POST", self.api_params) | ||
|
|
||
|
|
||
| class TelegramAPIPostOperator(TelegramAPIOperator): |
There was a problem hiding this comment.
Is this really necessary? Operators are built by compositions. The complex logic is in the hook. Inheritance does not bring benefits here.
There was a problem hiding this comment.
Same comment as above about reference implementation
There was a problem hiding this comment.
It will be difficult to make changes to the old code and keep it backwards compatible. However, if you have time, you can make a change, but remember to add a note in the file updating.md. Let us wait for the opinions of other people now.
| for retry in range(max_retries): | ||
| try: | ||
| return func(*args, **kwargs) | ||
| except (telegram.error.TimedOut,) as e: |
There was a problem hiding this comment.
Why only this error? What if an internal server error happens? Then you also need to retry.
dchaplinsky
changed the title
Co-Authored-By: dchaplinsky <chaplinsky.dmitry@gmail.com>
Co-Authored-By: dchaplinsky <chaplinsky.dmitry@gmail.com>
|
@dchaplinsky please update your PR description based on the original template (ref: #4890) You may find it tedious. But this make it easier for folks to understand what you're trying to propose in this PR, and easily link to JIRA. |
|
No problem I understand your intention to keep everything highly organized.
Little Q. Should I add hook/operator into contib section of API docs? If
so, I'll need to move imports of telegram bindings from global scope to
methods, otherwise documentation won't build without python-telegram-api
…On Mon, Mar 11, 2019 at 3:08 PM Xiaodong ***@***.***> wrote:
@dchaplinsky <https://github.com/dchaplinsky> please update your PR
description based on the original template (ref: #4890
<#4890>)
You may find it tedious. But this make it easier for folks to understand
what you're trying to propose in this PR, and easily link to JIRA.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcp-b_asHGDvLjtlDMK33Kqgzz-Dnks5vVlVRgaJpZM4bnTS4>
.
|
|
@dchaplinsky Look at: https://github.com/apache/airflow/blob/master/docs/conf.py#L39-L91 |
|
Ok, thanks. I'll probably add a howto guide on how to setup the private
channel and create a bot, cause I googling it myself every time. Operator
and hook are pretty straightforward.
…On Mon, Mar 11, 2019 at 6:30 PM Kamil Breguła ***@***.***> wrote:
@dchaplinsky <https://github.com/dchaplinsky> Look at:
https://github.com/apache/airflow/blob/master/docs/conf.py#L39-L91
You only need to add a library here and the documentation will build
correctly.
You should add class information to the code.rst file. If you have more
time then you can write a guide in the howto / operator directory, but
this is not required.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIActYEFRrEPeZGw2X5xIzRWvVKmtqgks5vVoSsgaJpZM4bnTS4>
.
|
|
I am now working on a new look for the documents from this section. I will be happy if you follow my new structure. http://purring-scent.surge.sh/howto/operator/gcp/natural_language.html This is not required. This is just my little request. |
|
Kamil, let me deliver updated PR first, because right now I feel like tasks
are arriving quicker than I can handle :)
…On Mon, Mar 11, 2019 at 6:38 PM Kamil Breguła ***@***.***> wrote:
I am now working on a new look for the documents from this section. I will
be happy if you follow my new structure.
http://purring-scent.surge.sh/howto/operator/gcp/natural_language.html
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcoUKR87JWrZpmYy4LGgUNnJC7eRnks5vVoZ1gaJpZM4bnTS4>
.
|
| Retry calling the decorated function using an exponential backoff. | ||
| (с) Eliot aka saltycrane, https://www.saltycrane.com/blog/2009/11/trying-out-retry-decorator-python/ | ||
|
|
||
| Args: |
There was a problem hiding this comment.
We use a reStructuredText docstring style.
| try: | ||
| return f(*args, **kwargs) | ||
| except exceptions as e: | ||
| msg = "{}, Retrying in {} seconds...".format(e, mdelay) |
There was a problem hiding this comment.
You should avoid formatting string before passing to logger.
Reference: #4804
|
That's copy-paste from existing solution which I updated with the copyright
and link.
I can rewrite it probably, but this function isn't exposed anywhere.
…On Mon, Mar 11, 2019 at 11:36 PM Kamil Breguła ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In airflow/contrib/hooks/telegram_hook.py
<#4891 (comment)>:
> @@ -0,0 +1,117 @@
+import telegram
+import time
+from functools import wraps
+from airflow.hooks.base_hook import BaseHook
+from airflow.exceptions import AirflowException
+
+
+def retry(exceptions, tries=4, delay=3, backoff=2, logger=None):
+ """
+ Retry calling the decorated function using an exponential backoff.
+ (с) Eliot aka saltycrane, https://www.saltycrane.com/blog/2009/11/trying-out-retry-decorator-python/
+
+ Args:
We use a reStructuredText docstring style.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcrrTJ-21BgyS2fYLQ5kJ4fhnKpesks5vVsxXgaJpZM4bnTS4>
.
|
|
Same comment as above.
…On Mon, Mar 11, 2019 at 11:37 PM Kamil Breguła ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In airflow/contrib/hooks/telegram_hook.py
<#4891 (comment)>:
> + tries: Number of times to try (not retry) before giving up.
+ delay: Initial delay between retries in seconds.
+ backoff: Backoff multiplier (e.g. value of 2 will double the delay
+ each retry).
+ logger: Logger to use. If None, print.
+ """
+
+ def deco_retry(f):
+ @wraps(f)
+ def f_retry(*args, **kwargs):
+ mtries, mdelay = tries, delay
+ while mtries > 1:
+ try:
+ return f(*args, **kwargs)
+ except exceptions as e:
+ msg = "{}, Retrying in {} seconds...".format(e, mdelay)
You should avoid formatting string before passing to logger.
Reference: #4804 <#4804>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcrylz2wV5tT85jc845EAPvSJ1vUBks5vVsypgaJpZM4bnTS4>
.
|
|
@dchaplinsky All elements from this module should be treated as public, because we want to have automatically generated documentation |
|
I.e I should just remove try/except block and not silence anything, so
it'll be at discretion of developer.
…On Mon, Mar 11, 2019 at 11:58 PM Kamil Breguła ***@***.***> wrote:
@dchaplinsky <https://github.com/dchaplinsky>
https://airflow.apache.org/concepts.html#trigger-rules
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcu8GuF93fTdX6u1tqKjxqp9xbtAvks5vVtGbgaJpZM4bnTS4>
.
|
|
@XD-DENG please check ticket and PR description |
|
@mik-laj , can we finish with code review part? I'd appreciate an answer on my question above, about retry decorator. |
| self.log.info(self.connection.send_message(**params)) | ||
|
|
||
|
|
||
| __all__ = [TelegramHook] |
There was a problem hiding this comment.
| __all__ = [TelegramHook] | |
| __all__ = ["TelegramHook"] |
This should be a list of module names, not modules.
However, I would like to point out that we do not use the all variable in the whole project. I think that this is a good rule because it forces you to think about the whole project, not just small fragments. In the future, when Airflow will be divided into modules and interfaces between libraries will be important, we can think about changing this rule. Now we have one code base and we must think about its overall good.
Why is the decorator only in this module, not in the entire Airflow? What if another operator needs this decorator? Copy code? Move code? If we copy to another place, what will we do with the documentation? Can we change it? Did we respect code style of author?
@zhongjiajie What are you thinking?
|
This is not enough reason to change these rules.I think that the code should be rewritten. We have rules that should be followed. If we accept this code, it will be rewritten in the future. We should avoid additional work in the future. Now this change is easier to make. |
|
Okay, no worries.
14 бер. 2019 р. о 04:16 Kamil Breguła <notifications@github.com> пише:
… This is not enough reason to change these rules.I think that the code should be rewritten. We have rules that should be followed. If we accept this code, it will be rewritten in the future. We should avoid additional work in the future. Now this change is easier to make.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@mik-laj , please review |
| "Cannot get chat_id: " "No valid chat_id nor telegram_conn_id supplied." | ||
| ) | ||
|
|
||
| @tenacity.retry( |
| "disable_web_page_preview": True, | ||
| } | ||
| params.update(api_params) | ||
| self.log.info(self.connection.send_message(**params)) |
There was a problem hiding this comment.
The full response should be at the debug level. At the info level, only the message that there is an attempt to send a message and that the message was sent.
|
I really like the solution with the tenacity library. ;-) Awesome! |
Co-Authored-By: dchaplinsky <chaplinsky.dmitry@gmail.com>
|
Updated
…On Mon, Mar 18, 2019 at 2:40 AM Kamil Breguła ***@***.***> wrote:
I really like the solution with the tenacity library. ;-) Awesome!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcvcWTsvJxiqO11Gfg5k7naZnFbw6ks5vXuBzgaJpZM4bnTS4>
.
|
|
@mik-laj can you check it now? |
|
I have a suggestion. To add it to your repository execute a command: curl https://termbin.com/wymk | git amCan you also add unit tests? |
|
Ok, will review and update repo shortly.
Automated tests — cannot promise that I can do it anytime soon :(
…On Sun, Mar 24, 2019 at 5:09 AM Kamil Breguła ***@***.***> wrote:
I have a suggestion. To add it to your repository execute a command:
curl https://termbin.com/wymk | git am
Can you also add automatic tests?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcmfzm7-Fch6VsEJ3AyYRE5QuDsO2ks5vZuxhgaJpZM4bnTS4>
.
|
|
Will look into it but quite limited in time right now and already spent on
that contrib way more time than I've planned.
…On Sun, Mar 24, 2019 at 11:53 AM Kamil Breguła ***@***.***> wrote:
Unit tests are needed only.
Example:
https://github.com/apache/airflow/blob/master/tests/contrib/operators/test_gcp_translate_operator.py
https://github.com/apache/airflow/blob/master/tests/contrib/hooks/test_gcp_translate_hook.py
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIAcnutdPPrN8FoOa3D_o07ygd7YPeAks5vZ0sQgaJpZM4bnTS4>
.
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
stale
|
Hi are there any plans to work further on this pull request? I would love to include this operator for notifications in my dags. Much easier than receiving tons of emails and sorting these emails out. |
|
Well, I gave up after two or three rounds of CR.
Don't have time to work on it and maintain it. If you wish you can finish
it and get it merged
…On Fri, Jun 5, 2020 at 5:31 PM GreenGrassBlueOcean ***@***.***> wrote:
Hi are there any plans to work further on this pull request? I would love
to include this operator for notifications in my dags. Much easier than
receiving tons of emails and sorting these emails out.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4891 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABAA4VNWN76XNI6VDKCPZTRVD6V3ANCNFSM4G45GS4A>
.
|
Make sure you have checked all steps below.
Jira
Description
Tests
Commits
Documentation
Code Quality
flake8