airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yingbo Wang <ybw...@gmail.com>
Subject Re: [VOTE] AIP-17: Consolidate and de-duplicate sensor tasks in airflow Smart Sensor
Date Sat, 20 Jun 2020 07:26:16 GMT
Thanks everyone for the feedback. I will also add the details mentioned in
this thread into the AIP


Q: From an implementation perspective, my one area of concern is the

"sharding" concept and the configuration / management overhead involved. I

may have missed it in the AIP, but would it be possible to add auto-scaling

to minimize this configuration?

The “sharding” configuration is an integer which implies the number of
concurrently running smart sensor jobs for the whole airflow cluster. A
proper sharding setting mainly depends on the following issues: 1. Cluster
load -- how many sensor tasks need to be executed at the same time. 2. How
often should each sensor be poked at least once. 3. The response time for a
sensor task in the current system. As these answers may vary for different
systems we leave “sharding” as a configurable field for users to satisfy
different use cases.

Also, a couple of clarifying questions:

1. Do you know if this is more suitable to certain kinds of sensors vs.

Others?

Most sensors should be suitable for the smart sensor. Except if the
argument needed to initialize a sensor object is unserializable, e.g. a
function. Serialize more complex types other than builtin and datetime is
not supported right now but we are planning to add them in the future.

2. What do you think about leveraging this to enable "async" operations

using Airflow i.e. submit a task and then use a "smart sensor" to check for

Completion?

This is a very good point. We do notice the relationship between these two
ideas. Technically this logic should also work. The “task submission” map
to the pre_execute() in a sensor task logic and “check for completion” map
to the sensor’s poke() function. The current implementation of
SubDagOperator
<https://github.com/apache/airflow/blob/master/airflow/operators/subdag_operator.py#L144-L177>
actually follows this pattern. If the operator requires no unserializable
argument to be instantiated, we should already be able to leverage the
async operation in SmartSensor for it.




Q: How would a user enable their own smart sensors? I don’t see any added
documentation for this. It looks like they need to manually add the name of
the class to the airflow configuration and do *something* to their sensor
class, including override the "is_smart_sensor" method (why a method and
not an attribute?)

Having to enable it in multiple places seems a little cumbersome, why not
have a "BaseSmartSensor" that the user inherits from like most of the rest
of Airflow? Sensors inherited from BaseSmartSensor would be "Smart" when
smart sensors are enabled in the configuration and not smart when smart
sensors are not enabled.

Enabling/Disabling the smart sensor is a system level config which is
transparent to the individual users. An example of smart sensor enabled
cluster config is as follows:

[smart_sensor]

use_smart_sensor = true

shard_code_upper_limit = 10000

shards = 5

sensor_enabled = NamedHivePartitionSensor, MetastorePartitionSensor


The "use_smart_sensor" config indicates if the smart sensor is enabled. The
"shards" config indicates the number of concurrently running smart sensor
jobs for the airflow cluster. The "sensor_enabled" config is a list of
sensor class names that will use the smart sensor.  The users use the same
class names (e.g. HivePartitionSensor) in their DAGs and they don’t have
the control to use smart sensors or not, unless they exclude their tasks
explicits.


Existing DAGs don't need to be changed for enabling/disabling the smart
sensor.


“Is_smart_sensor_compatible” is a class level configuration (instead of
instance-level) so that the system knows if a particular sensor operator
can use the smart sensor. Currently only NamedHivePartitionSensor and
MetastorePartitionSensor
are enabled to use the smart sensor in the PR.

To include other sensor operators for smart sensors that are not included
in this PR:

   1.

   Add a class attribute "poke_context_fields" to the operator.
   "poke_context_fields" include all key names used for initializing a sensor
   object.
   2.

   In airflow.cfg, add the operator’s classname to the session of
   “[smart_sensor]” with the field “sensors_enabled” as follows.


Yingbo

On Fri, Jun 19, 2020 at 7:27 AM Shaw, Damian P. <
damian.shaw.2@credit-suisse.com> wrote:

> Also +1 (non-binding) on the AIP but questions on the implementation.
>
> How would a user enable their own smart sensors? I don’t see any added
> documentation for this. It looks like they need to manually add the name of
> the class to the airflow configuration and do *something* to their sensor
> class, including override the "is_smart_sensor" method (why a method and
> not an attribute?)
>
> Having to enable it in multiple places seems a little cumbersome, why not
> have a "BaseSmartSensor" that the user inherits from like most of the rest
> of Airflow? Sensors inherited from BaseSmartSensor would be "Smart" when
> smart sensors are enabled in the configuration and not smart when smart
> sensors are not enaled.
>
> Damian
>
> -----Original Message-----
> From: Vikram Koka <vikram@astronomer.io>
> Sent: Friday, June 19, 2020 00:57
> To: dev@airflow.apache.org
> Subject: Re: [VOTE] AIP-17: Consolidate and de-duplicate sensor tasks in
> airflow Smart Sensor
>
> +1 (non-binding) for this AIP.
>
> I really like the concept and the efficiency improvements. The general
> SmartSensor concept and the ability to add additional sensor classes is
> elegant.
>
> From an implementation perspective, my one area of concern is the
> "sharding" concept and the configuration / management overhead involved. I
> may have missed it in the AIP, but would it be possible to add auto-scaling
> to minimize this configuration?
>
> Also, a couple of clarifying questions:
> 1. Do you know if this is more suitable to certain kinds of sensors vs.
> others?
> 2. What do you think about leveraging this to enable "async" operations
> using Airflow i.e. submit a task and then use a "smart sensor" to check for
> completion?
>
> Best regards,
>
> Vikram
>
>
>
>
> On Thu, Jun 18, 2020 at 3:38 PM Yingbo Wang <ybwang@gmail.com> wrote:
>
> > Hello everyone!
> >
> > This email calls for a vote to add the airflow smart sensor at
> > https://github.com/apache/airflow/pull/5499
> >
> > AIP:
> >
> > https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-17%3A+Consolid
> > ate+and+de-duplicate+sensor+tasks+in+airflow+Smart+Sensor
> >
> > Change summary:
> >
> >    - Add a new mode called “smart sensor mode”. In smart sensor mode,
> >    instead of holding a long running process for each sensor and poking
> >    periodically, a sensor will only store poke context at sensor_instance
> >    table and then exits with a ‘sensing’ state.
> >    - When the smart sensor mode is enabled, a special set of builtin
> smart
> >    sensor DAGs (named smart_sensor_group_shard_xxx) is created by the
> > system;
> >    These DAGs contain SmartSensorOperator task and manage the smart
> sensor
> >    jobs for the airflow cluster. The SmartSensorOperator task can fetch
> >    hundreds of ‘sensing’ instances from sensor_instance table and poke on
> >    behalf of them in batches. Users don’t need to change their
> > existing DAGs.
> >    - The smart sensor mode currently supports NamedHivePartitionSensor
> and
> >    MetastorePartitionSensor however it can easily be extended to
> > support more
> >    sensor classes.
> >    - Smart sensor mode on/off, the list of smart sensor enabled classes,
> >    and the number of SmartSensorOperator tasks can be configured in
> airflow
> >    config.
> >    - Sensor logs in smart sensors are populated to each task instance log
> >    UI.
> >
> >
> > A PR https://github.com/apache/airflow/pull/5499 is ready for review
> > from the committers and community.
> >
> >
> > This email is formally calling for a vote to accept the AIP and PR.
> > Please note that we will update the PR / feature to fix bugs if we find
> any.
> >
> >
> > Best
> >
> > Yingbo
> >
>
>
>
> ===============================================================================
>
> Please access the attached hyperlink for an important electronic
> communications disclaimer:
> http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html
> ===============================================================================
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message