Thanks Priya the FLIP now looks much better, I would move out some of the implementation details . Just highlight how the writer actually uses the client (the subitRequestEntries part) and how the Sink itself exposes its API to users (the Sink & builder part), other parts should not be part of the FLIP. Let's leave something for the coders ;).
I would still vote to add Table API support, you really would have done most of the work already, do you believe there is a specific concern not to include it? I am happy with the FLIP once reformatted. Thanks for the effort. Best Regards Ahmed Hamdy On Tue, 9 Apr 2024 at 23:59, Dhingra, Priya <dhipr...@amazon.com.invalid> wrote: > > > On 4/9/24, 3:57 PM, "Dhingra, Priya" <dhipr...@amazon.com.inva <mailto: > dhipr...@amazon.com.inva>LID> wrote: > > > CAUTION: This email originated from outside of the organization. Do not > click links or open attachments unless you can confirm the sender and know > the content is safe. > > > > > > > Hi Ahmed and Samrat, > > > Thanks a lot for all the feedbacks, this is my first ever contribution to > apache Flink, hence I was bit unaware about few things but updated all of > them as per your suggestions, thanks again for all the support here, much > appreciated!! > > > 1) I am not sure why we need to suppress warnings in the sink example in > the > FLIP? > > > Removed and updated the FLIP. > > > 2) You provided the sink example as it is the Public Interface, however the > actual AsyncSink logic is mostly in the writer, so would be helpful to > provide a brief of the writer or the "submitRequestEntries" > > > Added in FLIP > > > 3) I am not sure what customDimensions are or how are they going to be used > by the client, (that's why I thought the writer example should be helpful). > > > Removed. This is no more required, we have added in our code to support > some specific usecase, no more required for apache PR. > > > 4) Are we going to use the existing aws client providers to handle the > authentication and async client creation similar to Kinesis/Firehose and > DDB. I would strongly recommend we do. > > > Yes > > > 5) Given you suggest implementing this in "flink-connector-aws" repo, it > should probably follow the same versioning of AWS connectors, hence > targeting 4.3.0. Also I am not sure why we are targeting support for 1.16 > given that it is out of support and 4.2 supports 1.17+. > > > Sorry, was not aware about the versioning we should have it here. I tested > the sqs sink with flint 1.16 so thought of putting the same, but was not > aware about out of support. Updated now with 4.3.0 and 1.17+ > > > 6) Are we planning to support Table API & SQL as well? This should not be > much of an effort IMO so I think we should. > > > No, not putting that in scope for first iteration. We can take that as > fast follow up. > > > 7) It would be great if we also added an implementation of Element > converter given that SQS message bodies are mainly Strings if I remember > correctly. We can extend it to other types for MessageAttributeValue > augmentation,this should be more valuable on table API as well to use it as > default Element Converter. > > > Updated in FLIP > > > 8. Different connectors provide different types of fault > tolerant guarantees[1]. What type of fault tolerant sink guarantees > flink-connector-sqs will provide ? > Could you elaborate on the fault-tolerant capabilities that the > flink-connector-sqs will provide? > > > at-least-once > > > > > 9) Can you help with what the minimal configuration required for > instantiating the sink ? > > > SQSSink.builder() > .setSqsUrl(sqsUrl) > .setSqsClientProperties(getSQSClientProperties()) > .setSerializationSchema(serializationSchema) > .build(); > > > > > 10) Amazon SQS offers various data types [2]. Could you outline the types > of SQS data the sink plans to support? > > SendMessageBatchRequestEntry > > > > > > > > > > > > > > > Hi Priya, > > > > > Thank you for the FLIP. sqs connector would be a great addition to the > flink connector aws. > > > > > +1 for all the queries raised by Ahmed. > > > > > Adding to Ahmed's queries, I have a few more: > > > > > 1. Different connectors provide different types of fault > tolerant guarantees[1]. What type of fault tolerant sink guarantees > flink-connector-sqs will provide ? > Could you elaborate on the fault-tolerant capabilities that the > flink-connector-sqs will provide? > > > > > 2. Can you help with what the minimal configuration required for > instantiating the sink ? > > > > > 3. Amazon SQS offers various data types [2]. Could you outline the types of > SQS data the sink plans to support? > > > > > [1] > > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/ > < > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/> > < > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/> > < > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/> > ;> > [2] > > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html > < > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html> > < > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html> > < > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html> > ;> > > > > > Bests, > Samrat > > > > > On Sat, Apr 6, 2024 at 6:27 PM Ahmed Hamdy <hamdy10...@gmail.com <mailto: > hamdy10...@gmail.com> <mailto:hamdy10...@gmail.com <mailto: > hamdy10...@gmail.com>>> wrote: > > > > > > Hi Dhingra, thanks for raising the FLIP > > I am in favour of this addition in general. I have a couple of > > comments/questions on the FLIP. > > > > - I am not sure why we need to suppress warnings in the sink example in > the > > FLIP? > > - You provided the sink example as it is the Public Interface, however > the > > actual AsyncSink logic is mostly in the writer, so would be helpful to > > provide a brief of the writer or the "submitRequestEntries" > > - I am not sure what customDimensions are or how are they going to be > used > > by the client, (that's why I thought the writer example should be > helpful). > > - Are we going to use the existing aws client providers to handle the > > authentication and async client creation similar to Kinesis/Firehose and > > DDB. I would strongly recommend we do. > > - Given you suggest implementing this in "flink-connector-aws" repo, it > > should probably follow the same versioning of AWS connectors, hence > > targeting 4.3.0. Also I am not sure why we are targeting support for 1.16 > > given that it is out of support and 4.2 supports 1.17+. > > - Are we planning to support Table API & SQL as well? This should not be > > much of an effort IMO so I think we should. > > - It would be great if we also added an implementation of Element > converter > > given that SQS message bodies are mainly Strings if I remember correctly. > > We can extend it to other types for MessageAttributeValue augmentation, > > this should be more valuable on table API as well to use it as default > > Element Converter. > > > > I would love to assist with the implementation and reviews if this FLIP > was > > accepted. > > Best Regards > > Ahmed Hamdy > > > > > > On Fri, 5 Apr 2024 at 19:19, Dhingra, Priya <dhipr...@amazon.com.inva > <mailto:dhipr...@amazon.com.inva> <mailto:dhipr...@amazon.com.inva > <mailto:dhipr...@amazon.com.inva>>lid> > > wrote: > > > > > Hi Dev, > > > > > > I would like to start a discussion about FLIP-438: Amazon SQS Sink > > > Connector< > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > ;> > > >. > > > This FLIP is proposing to add support for AWS SQS sink in > > > flink-connector-aws repo. > > > > > > For more details, see FLIP-438. Looking forward to your feedback. > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > ;> > > > > > > Thanks, > > > Priya > > > > > > > > > > > > On 4/6/24, 9:07 AM, "Samrat Deb" <decordea...@gmail.com <mailto: > decordea...@gmail.com> <mailto:decordea...@gmail.com <mailto: > decordea...@gmail.com>>> wrote: > > > > > CAUTION: This email originated from outside of the organization. Do not > click links or open attachments unless you can confirm the sender and know > the content is safe. > > > > > > > > > > > > > Hi Priya, > > > > > Thank you for the FLIP. sqs connector would be a great addition to the > flink connector aws. > > > > > +1 for all the queries raised by Ahmed. > > > > > Adding to Ahmed's queries, I have a few more: > > > > > 1. Different connectors provide different types of fault > tolerant guarantees[1]. What type of fault tolerant sink guarantees > flink-connector-sqs will provide ? > Could you elaborate on the fault-tolerant capabilities that the > flink-connector-sqs will provide? > > > > > 2. Can you help with what the minimal configuration required for > instantiating the sink ? > > > > > 3. Amazon SQS offers various data types [2]. Could you outline the types of > SQS data the sink plans to support? > > > > > [1] > > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/ > < > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/> > < > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/> > < > https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/guarantees/> > ;> > [2] > > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html > < > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html> > < > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html> > < > https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_Types.html> > ;> > > > > > Bests, > Samrat > > > > > On Sat, Apr 6, 2024 at 6:27 PM Ahmed Hamdy <hamdy10...@gmail.com <mailto: > hamdy10...@gmail.com> <mailto:hamdy10...@gmail.com <mailto: > hamdy10...@gmail.com>>> wrote: > > > > > > Hi Dhingra, thanks for raising the FLIP > > I am in favour of this addition in general. I have a couple of > > comments/questions on the FLIP. > > > > - I am not sure why we need to suppress warnings in the sink example in > the > > FLIP? > > - You provided the sink example as it is the Public Interface, however > the > > actual AsyncSink logic is mostly in the writer, so would be helpful to > > provide a brief of the writer or the "submitRequestEntries" > > - I am not sure what customDimensions are or how are they going to be > used > > by the client, (that's why I thought the writer example should be > helpful). > > - Are we going to use the existing aws client providers to handle the > > authentication and async client creation similar to Kinesis/Firehose and > > DDB. I would strongly recommend we do. > > - Given you suggest implementing this in "flink-connector-aws" repo, it > > should probably follow the same versioning of AWS connectors, hence > > targeting 4.3.0. Also I am not sure why we are targeting support for 1.16 > > given that it is out of support and 4.2 supports 1.17+. > > - Are we planning to support Table API & SQL as well? This should not be > > much of an effort IMO so I think we should. > > - It would be great if we also added an implementation of Element > converter > > given that SQS message bodies are mainly Strings if I remember correctly. > > We can extend it to other types for MessageAttributeValue augmentation, > > this should be more valuable on table API as well to use it as default > > Element Converter. > > > > I would love to assist with the implementation and reviews if this FLIP > was > > accepted. > > Best Regards > > Ahmed Hamdy > > > > > > On Fri, 5 Apr 2024 at 19:19, Dhingra, Priya <dhipr...@amazon.com.inva > <mailto:dhipr...@amazon.com.inva> <mailto:dhipr...@amazon.com.inva > <mailto:dhipr...@amazon.com.inva>>lid> > > wrote: > > > > > Hi Dev, > > > > > > I would like to start a discussion about FLIP-438: Amazon SQS Sink > > > Connector< > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > ;> > > >. > > > This FLIP is proposing to add support for AWS SQS sink in > > > flink-connector-aws repo. > > > > > > For more details, see FLIP-438. Looking forward to your feedback. > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > < > https://cwiki.apache.org/confluence/display/FLINK/FLIP-438%3A+Amazon+SQS+Sink+Connector> > ;> > > > > > > Thanks, > > > Priya > > > > > > > > > > > > > > >
