Thank Shengkai for the new round of updating.
I don't have comments on the updates.
+1 for starting the vote.
Best,
Jark
On Thu, 19 May 2022 at 20:18, Shengkai Fang <fskm...@gmail.com> wrote:
> Hi, Timo.
>
> Thanks for your feedback!
>
> > SQLGatewayService.getFunction / UserDefinedFunctionInfo
>
> Yes. I miss some parts in the FLIP. I have fix the errors now.
>
> > configure_session
>
> Thanks for your inputs. Considering the difference, I am still prone to use
> the `configure_session`.
>
> > `./sql-gateway.sh`
>
> Yes, you are right. We should add a new startup script in the distribution.
> I update the FLIP and add the description to uses the script. Users can
> - start the server with `./sql-gateway.sh start -Dkey=value`
> - stop the last started server with `./sql-gateway.sh stop`
> - stop all the running server with `./sql-gateway.sh stop-all`
>
> > an "-e" converts a client to a server
> The "-e" options is just used to connect to the Gateway with the specified
> endpoint. It doesn't convert the client to the server.
>
> Thank everyone for all the inputs and discussion. If no other problems, I
> think we can restart the voting tomorrow.
>
> Best,
> Shengkai
>
>
> Timo Walther <twal...@apache.org> 于2022年5月19日周四 15:27写道：
>
> > Hi Shengkai,
> >
> > thanks again for the update. I don't have much to add:
> >
> > > I still think we should use a new state machine
> >
> > Thanks for the explanation about async/sync behavior. I thought we also
> > want to use the Gateway for job status updates. But if FINISHED only
> > refers to the job submission, the new state machine makes more sense.
> > Nevertheless, checking the job status will be a frequent request esp.
> > also for the new lifecycle statements. But I agree to the current design
> > that this is different to state of the operation.
> >
> > > SQLGatewayService.getFunction / UserDefinedFunctionInfo
> >
> > You forgot to update the class. There is still a UserDefinedFunctionInfo
> > in the FLIP.
> >
> > > configure_session
> >
> > I don't have a strong opinion on the naming of this particular REST
> > path. But my concern is that we should introduce a term of this family
> > of init/configure statements. Because in SQL Client we call it `init
> > statements` and in the gateway we call it `configuration statements`,
> > but in the end is all statements that are `not DML and DQL`.
> >
> > > './sql-client.h -e'
> >
> > Would it make sense to introduce a separate `./sql-gateway.sh`? I find
> > it a bit confusing that an "-e" converts a client to a server. Under the
> > hood we can still share the same classes but make it a bit more explicit
> > in the distribution (also for marketing purposes of this feature).
> >
> > Please continue with the voting afterwards.
> >
> > Regards,
> > Timo
> >
> > On 17.05.22 09:14, Shengkai Fang wrote:
> > > Hi, all.
> > >
> > > After discussing with Becket Qin offline, I modified the FLIP a little.
> > The
> > > change is as follow:
> > >
> > > 1. Add /api_versions in the REST API.
> > >
> > > The api is used by the client to know the current REST Endpoint
> supports
> > > which version. The client is able to choose the version for later
> > > communication.
> > >
> > > 2. SqlClient uses -e option to input the endpoint address and port.
> > >
> > > Because the -h option has already been used by the SqlClient. We use
> the
> > > -e, --endpoint for SqlClient to input the address:port of the endpoint.
> > >
> > > 3. Use './sql-client.h -e' to start the gateway mode rather than
> > > '/sql-client.h gateway -e'.
> > >
> > > If the user specifies the -e option, it definitely means to use the
> > gateway
> > > mode. Therefore, it is redundant to use another keyword to indicate the
> > > mode.
> > >
> > > Best,
> > > Shengkai
> > >
> > > Shengkai Fang <fskm...@gmail.com> 于2022年5月17日周二 14:13写道：
> > >
> > >> Hi, Jark, Timo. Nice to have an agreement!
> > >>
> > >> Thanks for Jark's inputs about the multiple version Flink. I have
> > already
> > >> updated the FLIP in the rejected alternatives about details.
> > >>
> > >> 1. We should definitely just use LogicalTypeJsonSerializer and not a
> > >> second JSON representation.
> > >>
> > >> Our concern is mainly that it's hard for users to use because of the
> > >> flexible structure. The LogicalTypeJsonSerializer will serialize the
> > >> VARCHAR to "VARCHAR(<LENGTH>)" or "{\"TYPE\": \"VARCHAR\", \"LENGTH\":
> > 0}",
> > >> which requires the end users to process the different situations. But
> in
> > >> some cases, users just print the json to the terminal/web UI. WDYT?
> > >>
> > >>> Serialize the RowData
> > >>
> > >> Sure. I will keep your advice in mind. I think the current
> serialization
> > >> of the RowData will not use the column name as the Object key in the
> > json.
> > >> I am not sure whether I missed something. It would be nice if you can
> > give
> > >> me an example if I do something wrong.
> > >>
> > >>> Have you also thought about using Flink's state types from Flink
> > >> tasks/jobs?
> > >>
> > >> Yes. But I still think we should use a new state machine. First of
> all,
> > >> Operation in the FLIP is much different from the Job. Operations
> include
> > >> DDL, DML and so on. So it's not suitable to use the small concept to
> > >> replace the big concept. Actually some status in the JobStatus, e.g.
> > >> RESTARTING/SUSPENDED/RECONCILING don't work in the DDL Operation.
> > >>
> > >> On the other hand, the Gateway allows users to submit jobs(DML) in
> > >> sync/async mode. The running status in the Operation Status in the
> > >> different mode has different meaning:
> > >> - In the async mode, when the gateway submits the job, the state comes
> > to
> > >> the FINISHED state
> > >> - In the sync mode, the running status in the Operation status
> includes
> > >> submitting the job, running job. Even if a failover occurs, we still
> > think
> > >> that this Operation is in the RUNNING state. Unless the job is
> > >> unrecoverable, we change the Operation status to ERROR.
> > >>
> > >> Therefore, I think these two concepts are not consistent and we should
> > not
> > >> reuse the JobStatus. I add a section in the rejected alternatives.
> > >>
> > >>> Options to configure the REST endpoint
> > >>
> > >> Yes. I have modified the FLIP about this.
> > >>
> > >>> Naming conversion
> > >>
> > >> Yes. I have modified the FLIP with your suggestions.
> > >>
> > >>> Another smaller shortcomings in the FLIP
> > >>
> > >>>> SQLGatewayService.getFunction / UserDefinedFunctionInfo
> > >>
> > >> After reviewing the java.sql.DatabaseMetaData#getFunctions's java
> doc, I
> > >> find it will return the system and user functions available in the
> > Catalog.
> > >> I think you are right. Therefore, we'd better to rename to the
> > >> listFunctions(SessionHandle sessionHandle, OperationHandle
> > operationHandle,
> > >> String catalog, String database, ShowFunctionsOperation.FunctionScope)
> > and
> > >> it returns FunctionInfo.
> > >>
> > >>>> SQLGatewayService.getGatewayInfo()/getSessionConfig
> > >>
> > >> The result of the SQLGatewayService.getGatewayInfo and
> getSessionConfig
> > is
> > >> not used by the endpoint. The endpoint just serializes the result and
> > >> presents it to the users. If we use the ReadableConfig, it's hard for
> > us to
> > >> iterate all the key value pairs.
> > >>
> > >>> configure_session VS initialize_session
> > >>>> If calling it initialize_session, should we limit it only being
> called
> > >> once?
> > >>
> > >> If we limit it only being called once, it allows the input of the
> > >> initialize_session script. But the current design in the Gateway is
> > aligned
> > >> with the TableEnvironment#executeSql. That is, the input of the
> > statement
> > >> is a single statement rather than the script. Considering the API in
> the
> > >> FLIP is not as same as the initialization in the CLI, I think we can
> use
> > >> the configure_session? What do you think, Timo?
> > >>
> > >> Best,
> > >> Shengkai
> > >>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >> Timo Walther <twal...@apache.org> 于2022年5月16日周一 14:28写道：
> > >>
> > >>> Hi Shengkai, Hi Jark,
> > >>>
> > >>> thanks for the additional explanation and the update of the FLIP.
> This
> > >>> will help us in the future for documenting our decisions. The
> arguments
> > >>> why to include the Gateway into the main repo make a lot of sense to
> > me.
> > >>> Esp. also because both CLI and gateway need some parsing
> functionality
> > >>> that is dependent on the current state of the SQL syntax.
> > >>>
> > >>> Here is my last set of feedback, other than that +1 for the proposal:
> > >>>
> > >>> Serialize the LogicalType
> > >>>
> > >>> The FLIP mentions LogicalTypeJsonSerializer but the shown JSON is
> > >>> different from the current master. We are using the serializable
> > >>> representation of LogicalType as much as possible nowadays. We should
> > >>> definitely just use LogicalTypeJsonSerializer and not a second JSON
> > >>> representation.
> > >>>
> > >>> 1) Serialize the RowData
> > >>>
> > >>> Side note for serializing ROWs: we should not use field names in JSON
> > >>> object keys. As e.g. `null` and other names with special characters
> > >>> cause issues in JSON.
> > >>>
> > >>> 2) We propose the state machine like HiveServer2
> > >>>
> > >>> Have you also thought about using Flink's state types from Flink
> > >>> tasks/jobs? If we were using Flink types directly, it would be easier
> > to
> > >>> monitor the execution of a INSERT INTO job via the gateway without
> > >>> having to map state types. Monitoring jobs is the most important
> > >>> functionality and should be in sync with regular Flink job
> monitoring.
> > A
> > >>> HiveServer2 endpoint can still perform mapping if necessary, but
> within
> > >>> the Flink code base we use a consistent state transition scheme.
> > >>>
> > >>> 3) Options to configure the REST endpoint
> > >>>
> > >>> Given that we also want to be able to put endpoint options into
> > >>> flink-conf.yaml, we should make those option a bit more globally
> > >>> understandable:
> > >>>
> > >>> endpoint.type -> sql-gateway.endpoint.type
> > >>> endpoint.rest.port -> sql-gateway.endpoint.rest.port
> > >>> ...
> > >>>
> > >>> 4) Naming conversion
> > >>>
> > >>> This is very nit picking, but please make sure to name the interfaces
> > >>> consistently for abbreviations. Currently, we mostly use
> `SqlInterface`
> > >>> instead of `SQLInterface`. So in the FLIP:
> > >>>
> > >>> SQLGatewayEndpoint -> SqlGatewayEndpoint
> > >>> SQLGatewayEndpointFactory -> SqlGatewayEndpointFactory
> > >>> e.g. for REST we do it already: RestEndpointVersion
> > >>>
> > >>> 5) Another smaller shortcomings in the FLIP
> > >>>
> > >>> SQLGatewayService.getFunction / UserDefinedFunctionInfo
> > >>> Was it a conscious decision to not use FunctionIdentifier here? You
> > will
> > >>> not be able to list system functions.
> > >>>
> > >>> SQLGatewayService.getGatewayInfo()/getSessionConfig
> > >>> Why not returning ReadableConfig here?
> > >>>
> > >>> Thanks,
> > >>> Timo
> > >>>
> > >>>
> > >>>
> > >>>
> > >>> On 12.05.22 17:02, Jark Wu wrote:
> > >>>> Hi Shengkai,
> > >>>>
> > >>>> One comment on the configure_session VS initialize_session,
> > >>>> I think configure_session is better than initialize_session,
> > >>>> because "initialize" sounds like this method can only be invoked
> once.
> > >>>> But I can see this method is useful to be invoked several times to
> > >>>> configure
> > >>>> the session during the lifecycle of a session. If calling it
> > >>>> initialize_session,
> > >>>> should we limit it only be called once?
> > >>>>
> > >>>> Best,
> > >>>> Jark
> > >>>>
> > >>>>
> > >>>>
> > >>>> On Thu, 12 May 2022 at 22:38, Jark Wu <imj...@gmail.com
> > >>>> <mailto:imj...@gmail.com>> wrote:
> > >>>>
> > >>>> Hi Timo,
> > >>>>
> > >>>> I agree with Shengkai that we should keep Gateway in the Flink
> > main
> > >>>> repo.
> > >>>> Here are my thoughts:
> > >>>>
> > >>>> 1) It is not feasible and maintainable to invoke different
> > versions
> > >>>> of Flink dependencies
> > >>>> in one JVM. I'm afraid it would be a nightmare when the code is
> > >>>> messed up with all the reflections.
> > >>>>
> > >>>> 2) Actually, it's very mature to support submitting
> > multi-version by
> > >>>> deploying one
> > >>>> gateway process for one version. Many systems are following
> this
> > >>>> architecture as Shengkai
> > >>>> mentioned above, including the Ververica Platform. And I don't
> > see
> > >>>> any problem with it.
> > >>>>
> > >>>> 3) As Jingsong said, SQL CLI should build on top of Gateway to
> > share
> > >>>> code and have the ability to
> > >>>> connect to Gateway. If SQL CLI is still kept in the Flink
> > repo, we
> > >>>> have to put Gateway there as well.
> > >>>>
> > >>>> 4) Besides, Gateway is indispensable for a SQL engine (think of
> > >>>> Trino/Presto, Spark, Hive).
> > >>>> Otherwise, Flink will always be a processing system. With
> Gateway
> > >>>> inside the Flink repo,
> > >>>> we can provide an out-of-box experience as a SQL query engine.
> > Users
> > >>>> can try out the gateway
> > >>>> for the latest version when a new version is released.
> > >>>>
> > >>>> 5) Last, Gateway inside the Flink repo can ensure the highest
> > degree
> > >>>> of version compatibility.
> > >>>>
> > >>>> Best,
> > >>>> Jark
> > >>>>
> > >>>>
> > >>>> On Thu, 12 May 2022 at 19:19, Shengkai Fang <fskm...@gmail.com
> > >>>> <mailto:fskm...@gmail.com>> wrote:
> > >>>>
> > >>>> Hi Timo.
> > >>>>
> > >>>> Thanks for your feedback!
> > >>>>
> > >>>> > It would be great if you can copy/paste some of the
> tricky
> > >>> design
> > >>>> decisions into the FLIP.
> > >>>>
> > >>>> Yes. I have already added a section about the `Rejected
> > >>>> Alternatives`. It
> > >>>> includes the discuss about the
> > >>>> - TableInfo and FunctionInfo VS CatalogTable and
> > CatalogFunction
> > >>>> - Support the multi-version Flink in the Gateway VS Support
> > the
> > >>>> multi-version in the external Service
> > >>>> - Merge Gateway into the Flink code base VS Support Gateway
> > in
> > >>>> the another
> > >>>> repo
> > >>>> ...
> > >>>>
> > >>>>
> > >>>> > Separate code base
> > >>>>
> > >>>> Let me summarize all the discussion about the separate code
> > >>>> base. Please
> > >>>> correct me if anything is wrong.
> > >>>>
> > >>>>
> > >>>> About support for the Gateway in the Flink code base.
> > >>>>
> > >>>>
> > >>>> 1. It reduces the cost including tests, release and
> > document. We
> > >>>> don't need
> > >>>> to upgrade the Gateway when Flink releases.
> > >>>> 2. The SQL Client has the ability to submit the SQL to the
> > >>>> Gateway. If we
> > >>>> move the Gateway to the outside repo, the SQL Client
> doesn't
> > >>>> have the
> > >>>> related dependencies.
> > >>>> 3. It is benefit for us to reuse code in the same repo.
> > >>>>
> > >>>> About supporting the Gateway in another repo.
> > >>>>
> > >>>> 1. It is easier to support the multi-version Flink;
> > >>>>
> > >>>>
> > >>>> First of all, I think we should support the multiple Flink
> > >>>> version with
> > >>>> another component that uses the Gateway from the individual
> > >>>> Flink releases.
> > >>>> Zeppelin, Livy, and Kyuubi all use similar architecture.
> > >>>>
> > >>>> The current Gateway in the FLIP is bound to the specific
> > Flink
> > >>>> version,
> > >>>> which needs to compile the SQL and submit the job to the
> > cluster
> > >>>> with the
> > >>>> specified Flink version.
> > >>>> We can introduce a service in the future(Maybe we can name
> it
> > >>>> MultipleVersionFlinkService? ). It may tell the client
> where
> > the
> > >>>> Gateway
> > >>>> with specified version is and the client just communicate
> > with
> > >>>> the Gateway
> > >>>> later. It may also just parse the request, convert the
> > request
> > >>>> to the REST
> > >>>> API in the FLIP and send to the Gateway with specified
> > version.
> > >>>>
> > >>>> Therefore, I think we should merge the SQL Gateway inside
> the
> > >>>> Flink repo.
> > >>>> The MultipleVersionFlinkService may be inside or outside
> the
> > >>> repo.
> > >>>>
> > >>>> I add more details in the `Rejected Alternatives` in the
> > FLIP.
> > >>>>
> > >>>> > Missing REST API spec
> > >>>>
> > >>>> Good point! I add a section in the `Appendix` about the
> > >>>> serialization of
> > >>>> the ResultSet. It includes how to serialize the schema,
> > RowData
> > >>> and
> > >>>> exceptions.
> > >>>>
> > >>>> > Consistency of concepts
> > >>>>
> > >>>> configure_session VS initialize_session
> > >>>>
> > >>>> Okay. I think it's better to use the initialize_session.
> > >>>>
> > >>>> TableInfo + UserDefinedFunctionInfo VS CatalogTable and
> > >>>> CatalogFunction
> > >>>>
> > >>>> The CatalogTable and CatalogFunction are much heavier than
> > the
> > >>>> TableInfo
> > >>>> and UserDefinedFunction. The CatalogManager requires
> reading
> > >>>> from the
> > >>>> Catalog to get the schema. But in the listTables only care
> > about
> > >>>> the table
> > >>>> name, which is much lighter. Therefore, we propose to use
> the
> > >>>> TableInfo
> > >>>> with required fields.
> > >>>>
> > >>>> > Result Retrieval
> > >>>>
> > >>>> Yes. Currently the API is only used for the preview. I
> added
> > a
> > >>>> section in
> > >>>> the `Rejected Alternatives` about this.
> > >>>>
> > >>>> Best,
> > >>>> Shengkai
> > >>>>
> > >>>
> > >>>
> > >
> >
> >
>
