Hi Kui,
Thanks for adding it into the Flip. There is one more thing wrt this topic
you might want to pay attention to, a little bit off-topic, is that Flink
SQL users might not be familiar with use cases of low level Datastream API.
IMHO, it is highly recommended(mandatory) to write the dependency you just
described in your last email and all related information in Flink doc
during developing this FLIP within the upcoming PR. Without those
guidelines in doc, users might feel confused after using those OPTIONs,
since they might not be aware of what happens underneath, and therefore
don't know why it does not work even if they did everything right at Flink
SQL level.
Best regards,
Jing
On Tue, Mar 7, 2023 at 6:36 AM Kui Yuan <catye...@gmail.com> wrote:
Hi Jing,
Thanks for the reminder. The aim of this flip is letting the sql users to
use those features in the Datastream API, we don't intend to extend
flip-217. In my opinion, the watermark alignment with only one source can
be configured by the options given in flip, and if the source connector
does not implement flip-217, the task will run with an error, reminding
the
user to use `pipeline.watermark-alignment.allow-
unaligned-source-splits`,
but maybe these behaviors are not understood by everyone, I will add some
tips about flip-217 in the flip to let users understand the behavior in
the
case of source splits.
Best,
Kui Yuan
Jing Ge <j...@ververica.com.invalid> 于2023年3月7日周二 04:23写道：
Hi Kui,
Thanks for pointing that out. I knew FLIP-217 which was done by an
engineer working in my team. As far as I am concerned, your FLIP
should
answer the following questions:
1. How to enable the watermark alignment of a source splits with Flink
SQL?
e.g. which options should be used if only one source is used?
2. Default behaviour. i.e. Flink SQL users should be aware that
watermark
alignment of source split will only work for sources that implement
FLIP-217 properly. Should users take care of
`pipeline.watermark-alignment.allow-unaligned-source-splits`
while using Flink SQL?
Best regards,
Jing
On Fri, Mar 3, 2023 at 8:46 AM Kui Yuan <catye...@gmail.com> wrote:
Hi all,
Thanks for all. There are more questions and I will answer one by
one.
@Jark Thanks for your tips. For the first question, I will add more
details
in the flip, and give a POC[1] so that pepole can know how I'm
currently
implementing these features.
IIRC, this is the first time we introduce the framework-level
connector
options that the option is not recognized and handled by
connectors.
The FLIP should cover how framework filters the watermark related
options
to avoid discover connector factory failed, and what happens if the
connector already supported the conflict options
For the second question, We know that the default strategy is
'on-periodic'
in SQL layer, and the default interval is 200ms. The reason for
emiting
watermark periodically is that the time advancement of consecutive
events
may be very small, we don't need to calculate watermark for each
event.
Same for 'on-event' strategy, so my idea is that we can set a fixed
gap
for
'on-event' strategy.
I'm not sure about the usage scenarios of event gap emit strategy.
Do
you have any specific use case of this strategy? I'm confused why
no
one
requested this strategy before no matter in DataStream or SQL, but
maybe
I missed something. I'm not against to add this option, but just
want
to
be
careful when adding new API because it's hard to remove in the
future.
As @Timo said, There is no default features like 'on-event-gap' in
DataStream API, but the users can achieve the 'on-event-gap' feature
by
using `WatermarkGenerator` interface, just like the implemention in
my
POC[1]. However, If we don't provide it in SQL layer, there is no
way
for
users to use similar features.
Jark raised a very good point. I thought we only expose what is
contained in DataStream API already. If this strategy is not part
of
DataStream API, would like to exclude it from the FLIP. We need to
be
careful which strategies we offer by default.
@Jark @Timo I'm sorry, perhaps I don't understand what are your
concerns
about CompiledPlan, maybe I missed something else, maybe you can look
at
my
POC first to see if there is somewhere to worry about.
Sorry, I forgot to remind you that Timo's concern about the changes
to
the
CompiledPlan looks like is still not covered in the FLIP.
@Jing We could have more discussion about naming, but I prefer that
the
naming should be consistent with the DataStream API.
About aligning splits/partitions/shards, maybe you missed FLIP-217[2]
which
aims to support watermark alignment of source splits.
After reading the most up-to-date Flip, I didn't find any
information
if
this solution will support aligning splits/partitions/shards [1].
Did I
miss anything?
Best
Kui Yuan
[1] the POC:
https://github.com/yuchengxin/flink/tree/yuankui/watermark_params
[2] FLIP-217:
https://cwiki.apache.org/confluence/display/FLINK/FLIP-217%3A+Support+watermark+alignment+of+source+splits
Jing Ge <j...@ververica.com.invalid> 于2023年3月3日周五 08:03写道：
Hi,
Thanks Kui for driving this Flip and thanks all for the informative
discussion.
@Timo
Your suggestion about the naming convention is excellent. Thanks! I
was
wondering why you, exceptionally, suggested 'scan.idle-timeout'
instead
of
'scan.watermark.idle-timeout'. I must miss something here.
There is one more NIT. I am just aware that "drift" is used for the
watermark alignment. It seems to be fine while using DataStream
API,
because we will not really see it. But with the OPTIONS in SQL, a
much
bigger group of users (including SRE, tech support, etc) will see
the
word
"drift". Given that "drift" wasn't used widely yet and with all
training
materials, Flink doc [1][2][3] (search with "lag"), "lag" has been
used
to
describe timestamp difference between watermark and its
corresponding event. Do we really need to introduce another term
for
the
same thing? How about using
'scan.watermark.alignment.max-lag'='1min'
and
change the parameter name from maxAllowedWatermarkDrift to
maxAllowedWatermarkLag [4] because of naming consistency? Just my
two
cents
worth.
@Kui
After reading the most up-to-date Flip, I didn't find any
information
if
this solution will support aligning splits/partitions/shards [1].
Did I
miss anything?
+1 for the concern about Table API. We'd be better keep Table API
and
SQL
synced for new features.
Best regards,
Jing
[1]
https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/dev/datastream/event-time/generating_watermarks/#watermark-alignment-_beta_
[2]
https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/dev/datastream/event-time/built_in/#fixed-amount-of-lateness
[3]
https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/connectors/datastream/kafka/
[4]
https://github.com/apache/flink/blob/4aacff572a9e3996c5dee9273638831e4040c767/flink-core/src/main/java/org/apache/flink/api/common/eventtime/WatermarkStrategy.java#L169
On Wed, Mar 1, 2023 at 3:54 PM Timo Walther <twal...@apache.org>
wrote:
Reg. 2:
> event gap emit strategy [...] no matter in DataStream or SQL
Jark raised a very good point. I thought we only expose what is
contained in DataStream API already. If this strategy is not part
of
DataStream API, would like to exclude it from the FLIP. We need
to
be
careful which strategies we offer by default.
Reg 1:
This already has a JIRA ticket with additional thoughts on this
topic:
https://issues.apache.org/jira/browse/FLINK-25221
Regards,
Timo
On 01.03.23 12:31, Jark Wu wrote:
Sorry, I forgot to remind you that Timo's concern about the
changes
to
the
CompiledPlan looks like is still not covered in the FLIP.
Best,
Jark
On Wed, 1 Mar 2023 at 19:28, Jark Wu <imj...@gmail.com> wrote:
Hi Kui,
Thank you for the great proposal, I think this is already in a
good
shape.
Just a kind reminder, according to the community
guidelines[1],
if there are unresponsive reviewers, a typical reasonable time
to wait for responses is one week, but be pragmatic about it.
Regarding the FLIP, I have some comments below:
1. IIRC, this is the first time we introduce the
framework-level
connector
options that the option is not recognized and handled by
connectors.
The FLIP should cover how framework filters the watermark
related
options
to avoid discover connector factory failed, and what happens
if
the
connector
already supported the conflict options.
2. I'm not sure about the usage scenarios of event gap emit
strategy.
Do
you have any specific use case of this strategy? I'm confused
why
no
one
requested this strategy before no matter in DataStream or SQL,
but
maybe
I missed something. I'm not against to add this option, but
just
want
to
be
careful when adding new API because it's hard to remove in the
future.
3. Adding a "Public Interface"[2] section to summarize the
proposed APIs and options would be better for developers to
know the impact. Currently, the APIs are scattered in the long
design sections.
Best,
Jark
[1]:
https://cwiki.apache.org/confluence/display/FLINK/Flink+Improvement+Proposals
[2]:
https://cwiki.apache.org/confluence/display/FLINK/FLIP+Template
On Wed, 1 Mar 2023 at 16:56, Kui Yuan <catye...@gmail.com>
wrote:
Hi all,
Thanks for all discussions!
Anyone else have questions or suggestions? if not, I will
start a
vote
thread later.
Best
Kui Yuan
kui yuan <catye...@gmail.com> 于2023年2月27日周一 20:21写道：
Hi Timo,
Thanks for your advice. I totally agree with your suggestion
of
naming
convention, I will rename these options and update the flip
later,
thanks
very much.
In our internal implementation we had put these options
inside
the
`FactoryUtil`, just as you expect. We have also taken into
account
the
changes to the CompiledPlan and we have packaged these
options
appropriately to minimize intrusiveness and ensure the
compatibility
to
the
`WatermarkPushDownSpec`.
A hint to the implementation: I would suggest that we add
those
options
to `FactoryUtil`. All cross-connector options should end up
there.
Please also consider the changes to the CompiledPlan in
your
FLIP.
This
change has implications on the JSON format as watermark
strategy
of
ExecNode becomes more complex, see WatermarkPushDownSpec
Best
Kui Yuan
Timo Walther <twal...@apache.org> 于2023年2月27日周一 18:05写道：
Hi Kui Yuan,
thanks for working on this FLIP. Let me also give some
comments
about
the proposed changes.
I support the direction of this FLIP about handling these
watermark-specific properties through options and
/*+OPTIONS(...)
*/
hints.
Regarding naming, I would like to keep the options in sync
with
existing
options:
> 'watermark.emit.strategy'='ON_EVENT'
Let's use lower case (e.g. `on-event`) that matches with
properties
like
sink.partitioner [1] or sink.delivery-guarantee [2].
> 'source.idle-timeout'='1min'
According to FLIP-122 [3], we want to prefix all
scan-source
related
properties with `scan.*`. This clearly includes
idle-timeout
and
actually also watermark strategies which don't apply for
lookup
sources.
Summarizing the comments above, we should use the following
options:
'scan.watermark.emit.strategy'='on-event',
'scan.watermark.emit.on-event.gap'='10000',
'scan.idle-timeout'='1min',
'scan.watermark.alignment.group'='alignment-group-1',
'scan.watermark.alignment.max-drift'='1min',
'scan.watermark.alignment.update-interval'='1s'
I know that this makes the keys even longer, but given that
those
options are for power users this should be acceptable. It
also
clearly
indicates which options are for sinks, scans, and lookups.
This
potentially also helps in allow lists.
A hint to the implementation: I would suggest that we add
those
options
to `FactoryUtil`. All cross-connector options should end up
there.
Please also consider the changes to the CompiledPlan in
your
FLIP.
This
change has implications on the JSON format as watermark
strategy
of
ExecNode becomes more complex, see WatermarkPushDownSpec
[4].
Regards,
Timo
[1]
https://nightlies.apache.org/flink/flink-docs-release-1.11/dev/table/connectors/kafka.html#sink-partitioner
[2]
https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/connectors/table/kafka/#sink-delivery-guarantee
[3]
https://cwiki.apache.org/confluence/display/FLINK/FLIP-122%3A+New+Connector+Property+Keys+for+New+Factory
[4]
https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/abilities/source/WatermarkPushDownSpec.java
On 24.02.23 04:55, kui yuan wrote:
Hi all,
I have updated the flip according to the discussion, and
we
will
extend
the
watermark-related features with both table options and
'OPTIONS'
hint,
like
this:
```
-- configure in table options
CREATE TABLE user_actions (
...
user_action_time TIMESTAMP(3),
WATERMARK FOR user_action_time AS user_action_time -
INTERVAL
'5'
SECOND
) WITH (
'watermark.emit.strategy'='ON_PERIODIC',
...
);
-- use 'OPTIONS' hint
select ... from source_table /*+
OPTIONS('watermark.emit.strategy'=
'ON_PERIODIC') */
```
Does everybody have any other questions?
Best
Kui Yuan
kui yuan <catye...@gmail.com> 于2023年2月23日周四 20:05写道：
Hi all,
Thanks for all suggestions.
We will extend the watermark-related features in SQL
layer
with
dynamic
table options and 'OPTIONS' hint, just as everyone
expects. I
will
modify
Flip-296 as discussed.
@Martijn As far as I know, there is no hint interface in
the
table
API,
so we can't use hint in table API directly. if we need to
extend
the
hint
interface in the table API, maybe we need another flip.
However,
if
we
extend the watermark-related features in the dynamic
table
options,
maybe
we are able to use them indirectly in the table API like
this[1]:
```
// register a table named "Orders"
tableEnv.executeSql("CREATE TABLE Orders (`user` BIGINT,
product
STRING,
amount INT) WITH
('watermark.emit.strategy'='ON_EVENT'...)");
```
[1]
https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/sql/create/
Best
Kui Yuan
Yun Tang <myas...@live.com> 于2023年2月23日周四 17:46写道：
Thanks for the warm discussions!
I had an offline discussion with Kui about the replies.
I
think
I
could
give some explanations on the original intention to
introduce
another
WATERMARK_PARAMS. If we take a look at the current
datastream
API,
the
watermark strategy does not belong to any specific
connector.
And
we
thought the dynamic table options were more like the
configurations
within
some specific connector.
From the review comments, I think most people feel
good
to
make
it
part
of the dynamic table options. I think this is fine if we
give
more
clear
scope definition of the dynamic table options here. And
I
also
agree
with
Jingsong's concern about adding SQL syntax which is the
most
concerning
part before launching this discussion.
For Martijn's concern, if we accept to make the
watermark-related
options
part of dynamic table options, the problem becomes
another
topic:
how
to
support the dynamic table options in table API, which is
deserved
to
create
another FLIP.
Best
Yun Tang
________________________________
From: Martijn Visser <martijnvis...@apache.org>
Sent: Thursday, February 23, 2023 17:14
To: dev@flink.apache.org <dev@flink.apache.org>
Subject: Re: [DISCUSS] FLIP-296: Watermark options for
table
API &
SQL
Hi,
While I can understand that there's a desire to first
focus
on
solving
this
problem for SQL, I do wonder if we should ignore the
Table
API
at
this
point. If we could include the syntax for the Table API,
it
potentially
could also be implemented by another contributor without
needing
to
create
another FLIP. If we don't design it right now, my
concern
is
that
this
will
increase sparsity for the Table API which ultimately
hurts
adoption.
With regards to the syntax, I have a preference to solve
this
via
the
connector options (e.g. like you can currently specify
things
as
scan.startup.specific-offsets or scan.bounded.mode for
the
Kafka
connector). You could still use the dynamic table
options
to
override/add
them.
Best regards,
Martijn
On Thu, Feb 23, 2023 at 7:21 AM Shammon FY <
zjur...@gmail.com
wrote:
Hi kui
Thanks for your answer and +1 to yuxia too
we should not bind the watermark-related options to a
connector
to
ensure
semantic clarity.
In my opinion, adding watermark-related options to a
connector
is
much
more
clear. Currently users can define simple watermark
strategy
in
DDL,
adding
more configuration items in connector options is easy
to
understand
Best,
Shammon
On Thu, Feb 23, 2023 at 10:52 AM Jingsong Li <
jingsongl...@gmail.com
wrote:
Thanks for your proposal.
+1 to yuxia, consider watermark-related hints as
option
hints.
Personally, I am cautious about adding SQL syntax,
WATERMARK_PARAMS
is
also SQL syntax to some extent.
We can use OPTIONS to meet this requirement if
possible.
Best,
Jingsong
On Thu, Feb 23, 2023 at 10:41 AM yuxia <
luoyu...@alumni.sjtu.edu.cn
wrote:
Hi, Yuan Kui.
Thanks for driving it.
IMO, the 'OPTIONS' hint may be not only specific to
the
connector
options. Just as a reference, we also have
`sink.parallelism`[1]
as
a
connector options. It enables
user to specific the writer's parallelism dynamically
per-query.
Personally, I perfer to consider watermark-related
hints
as
option
hints. So, user can define a default watermark
strategy
for
the
table,
and
if user dosen't needed to changes it, they need to do
nothing
in
their
query instead of specific it ervery time.
[1]
https://nightlies.apache.org/flink/flink-docs-master/zh/docs/connectors/table/filesystem/#sink-parallelism
Best regards,
Yuxia
Best regards,
Yuxia
----- 原始邮件 -----
发件人: "kui yuan" <catye...@gmail.com>
收件人: "dev" <dev@flink.apache.org>
抄送: "Jark Wu" <imj...@gmail.com>
发送时间: 星期三, 2023年 2 月 22日 下午 10:08:11
主题: Re: [DISCUSS] FLIP-296: Watermark options for
table
API &
SQL
Hi all,
Thanks for the lively discussion and I will respond
to
these
questions
one
by one. However, there are also some common questions
and I
will
answer
together.
@郑 Thanks for your reply. The features mentioned in
this
flip
are
only
for
those source connectors that implement the
SupportsWatermarkPushDown
interface, generating watermarks in other graph
locations
is
not in
the
scope of this discussion. Perhaps another flip can be
proposed
later to
implement this feature.
@Shammon Thanks for your reply. In Flip-296, a
rejected
alternative
is
adding watermark related options in the connector
options，we
believe
that
we should not bind the watermark-related options to a
connector
to
ensure
semantic clarity.
What will happen if we add watermark related options
in
`the
connector
options`? Will the connector ignore these options or
throw
an
exception?
How can we support this?
If user defines different watermark configurations
for
one
table in
two
places, I tend to prefer the first place would
prevail,
but
we
can
also
throw exception or just print logs to prompt the
user,
which
are
implementation details.
If one table is used by two operators with different
watermark
params,
what will happen?
@Martijn Thanks for your reply. I'm sorry that we
are
not
particularly
accurate, this hint is mainly for SQL, not table
API.
While the FLIP talks about watermark options for
Table
API &
SQL,
I
only
see proposed syntax for SQL, not for the Table API.
What
is
your
proposal
for the Table API
@Jane Thanks for your reply. For the first question,
If
the
user
uses
this
hint on those sourse that does not implement the
SupportsWatermarkPushDown
interface, it will be completely invalid. The task
will
run
as
normal
as
if
the hint had not been used.
What's the behavior if there are multiple table
sources,
among
which
some do not support `SupportsWatermarkPushDown`?
@Jane feedback that 'WATERMARK_PARAMS' is difficult
to
remember,
perhaps
the naming issue can be put to the end of the
discussion,
because
more
people like @Martijn @Shuo are considering whether
these
configurations
should be put into the DDL or the 'OPTIONS' hint.
Here's
what I
think, Putting these configs into DDL or putting them
into
'OPTIONS'
hint
is actually the same thing, because the 'OPTIONS'
hint
is
mainly
used
to
configure the properties of conenctor. The reason
why I
want
to
use
a
new
hint is to make sure the semantics clear, in my
opinion
the
configuration
of watermark should not be mixed up with connector.
However,
a
new
hint
does make it more difficult to use to some extent,
for
example,
when a
user
uses both 'OPTIONS' hint and 'WATERMARK_PARAMS' hint.
For
this
point,
maby
it is more appropriate to use uniform 'OPTIONS' hint.
On the other hand, if we enrich more watermark option
keys
in
'OPTIONS'
hints, The question will be what we treat the
definatrions
of
'OPTIONS'
hint, is this only specific to the connector options
or
could
be
more?
Maybe @Jark could share more insights here. In my
opion,
'OPTIONS'
is
only
related to the connector options, which is not like
the
gernal
watermark
options.
Shuo Cheng <njucs...@gmail.com> 于2023年2月22日周三
19:17写道：
Hi Kui,
Thanks for driving the discussion. It's quite useful
to
introduce
Watermark
options. I have some questions:
What kind of hints is "WATERMARK_PARAMS"?
Currently, we have two kinds of hints in Flink:
Dynamic
Table
Options &
Query Hints. As described in the Flip,
"WATERMARK_PARAMS"
is
more
like
Dynamic Table Options. So two questions arise here:
1) Are these watermark options to be exposed as
connector
WITH
options? Aa
described in SQL Hints doc[1], "Dynamic Table
Options
allow
to
specify or
override table options dynamically", which implies
that
these
options
can
also be configured in WITH options.
2) Do we really need a new hint name like
'WATERMARK_PARAMS',
table
options use "OPTIONS" as hint name, like '/*+
OPTIONS('csv.ignore-parse-errors'='true') */', maybe
we
can
enrich
more
table option keys for watermark, e.g., /*+
OPTIONS('watermark.emit-strategy'='ON_PERIODIC') */.
[1]
https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/dev/table/sql/queries/hints/
On Wed, Feb 22, 2023 at 10:22 AM kui yuan <
catye...@gmail.com
wrote:
Hi devs,
I'd like to start a discussion thread for
FLIP-296[1].
This
comes
from an
offline discussion with @Yun Tang, and we hope to
enrich
table
API
&
SQL
to
support many watermark-related features which were
only
implemented
at
the
datastream API level.
Basically, we want to introduce watermark options
in
table
API &
SQL
via
SQL hint named 'WATERMARK_PARAMS' to support
features:
1、Configurable watermark emit strategy
2、Dealing with idle sources
3、Watermark alignment
Last but not least, thanks to Qingsheng and Jing
Zhang
for
the
initial
reviews.
Looking forward to your thoughts and any feedback
is
appreciated!
[1]
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=240884405
Best
Yuan Kui
