Skip to content
This repository has been archived by the owner on Dec 14, 2022. It is now read-only.

Support protobuf with flink #255

Merged

Conversation

jianyun8023
Copy link
Contributor

Support based on Flink's protobuf version contribution implementation.
The Flink format protobuf is in the process of being merged. New Format of protobuf

@jianyun8023 jianyun8023 requested review from reswqa and a team as code owners February 26, 2021 01:32
@jianyun8023 jianyun8023 self-assigned this Feb 26, 2021
@jianyun8023
Copy link
Contributor Author

temporarily put the pb-format code into this repository, and will remove the pb-format code from this repository when pb-format is released.

@jianyun8023 jianyun8023 changed the title [WIP] Support protobuf with flink Support protobuf with flink Mar 8, 2021
@jianyun8023
Copy link
Contributor Author

There are still two issues to be considered:

  • ClassLoader
    In sql mode, should I use the user classloader or the connector classloader,
    How to use it in catalog mode?

  • Dependencys
    How to handle the dependency of the connector on protobuf? How do I get the user to reintroduce protobuf when needed?

@jianyun8023
Copy link
Contributor Author

The files under flink-protobuf can be ignored during review, it is only temporarily introduced. The flink-protobuf directory will be removed when it has available jars.

@jianyun8023
Copy link
Contributor Author

@hnail Can you review it for me?

@lgtm-com
Copy link

lgtm-com bot commented Mar 11, 2021

This pull request introduces 1 alert when merging a7a1b4a into aa3a990 - view on LGTM.com

new alerts:

  • 1 for Missing space in string literal

@lgtm-com
Copy link

lgtm-com bot commented Mar 11, 2021

This pull request introduces 1 alert when merging 97604fd into aa3a990 - view on LGTM.com

new alerts:

  • 1 for Missing space in string literal

Copy link
Contributor

@hnail hnail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

  • I agreed the current implementation based on Flink's protobuf version contribution implementation which dependent on protobuf GeneratedMessageV3 implementation class is cost-effective and efficient. The current implementation can support pulsar SchemaType.PROTOBUF_NATIVE and SchemaType.PROTOBUF schema .
  • Class depend problem which you mentioned can be reslove with Pulsar's SchemaType.PROTOBUF_NATIVE Schema which support get Descriptor from pulsar schema registry , additional optimize may need :
    1. PbFormatUtils.getDescriptor() support get Descriptor from pulsar schema registry;
    2. SchemaUtils.tableSchemaToSchemaInfo() support get Descriptor from pulsar schema registry;

if (valueSerialization instanceof PbRowDataSerializationSchema) {
try {
String messageClassName =
(String) FieldUtils.readDeclaredField(valueSerialization, "messageClassName", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"messageClassName" declare as static final class-field ?

Copy link
Contributor Author

@jianyun8023 jianyun8023 Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it can be declared as final.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the schema cache so that this method will only be called once

@lgtm-com
Copy link

lgtm-com bot commented Mar 18, 2021

This pull request introduces 1 alert when merging 9169ef5 into aa3a990 - view on LGTM.com

new alerts:

  • 1 for Missing space in string literal

# Conflicts:
#	pom.xml
#	pulsar-flink-connector/pom.xml
#	pulsar-flink-connector/src/main/java/org/apache/flink/streaming/connectors/pulsar/internal/PulsarCatalogSupport.java
#	pulsar-flink-connector/src/main/java/org/apache/flink/streaming/connectors/pulsar/internal/SchemaUtils.java
@lgtm-com
Copy link

lgtm-com bot commented Mar 19, 2021

This pull request introduces 1 alert when merging 4a008cf into 5696e81 - view on LGTM.com

new alerts:

  • 1 for Missing space in string literal

@jianyun8023 jianyun8023 merged commit 9c44d53 into streamnative:master Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants