Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-10769][Table & SQL] port InMemoryExternalCatalog to java #7012

Closed
wants to merge 2 commits into from
Closed

[FLINK-10769][Table & SQL] port InMemoryExternalCatalog to java #7012

wants to merge 2 commits into from

Conversation

bowenli86
Copy link
Member

@bowenli86 bowenli86 commented Nov 3, 2018

What is the purpose of the change

In the Flink-Hive integration design, we propose a new FlinkInMemoryCatalog (FLINK-10697) for production use, unlink InMemoryExternalCatalog which is only used for testing and dev work. FlinkInMemoryCatalog will share some part with the existing InMemoryExternalCatalog, thus we need to make changes to InMemoryExternalCatalog.

Background: there are two parallel efforts going on right now - FLINK-10686, driven by Timo, includes moving external catalogs APIs from flink-table to flink-table-common, also from Scala to Java; FLINK-10744 I'm working on right now to integrate Flink with Hive and enhance external catalog functionality.

As discussed with @twalthr in FLINK-10689, we'd better parallelize these efforts while introducing minimal overhead for integrating them later. Our agreed way is to writing new code/feature related to external catalogs/hive in Java in flink-table first then move to other module like flink-table-common, this way we can minimize migration efforts. If existing classes are modified for a feature we can start migrating them to Java in a separate commit first and then perform the actual feature changes, and migrated classes can be placed in flink-table/src/main/java until we find a better module structure.

Therefore, we will port InMemoryExternalCatalog to java first. This PR only port scala to java with NO feature or behavior change. This is a pre-requisite for FLINK-10697

Brief change log

port InMemoryExternalCatalog and InMemoryExternalCatalogTest to java

Verifying this change

This change is already covered by existing tests, such as InMemoryExternalCatalog.

Does this pull request potentially affect one of the following parts:

none

Documentation

  • Does this pull request introduce a new feature? (no)

@bowenli86 bowenli86 changed the title [FLINK-10769] port InMemoryExternalCatalog to java [FLINK-10769][Table & SQL] port InMemoryExternalCatalog to java Nov 3, 2018
import java.util.Map;

/**
* This class is an in-memory implementation of [[ExternalCatalog]].
Copy link
Member

Choose a reason for hiding this comment

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

{@link ExternalCatalog}?

@bowenli86
Copy link
Member Author

bowenli86 commented Nov 5, 2018

@twalthr @fhueske @xuefuz Can you please take a look?

@fhueske
Copy link
Contributor

fhueske commented Nov 5, 2018

Thanks for the PR @bowenli86. I haven't had a detailed look at the PR yet (will try to do in the next days).

Just one side note. I don't think there we have consensus on porting all code of the flink-table module to Java. In fact, I'd really like to avoid a fragmented code base. Instead, I'm in favor of splitting the module into modules (for example flink-table-runtime, flink-table-planning, flink-table-api) that can be incrementally ported to Java. This should be fairly easy for flink-table-runtime and hopefully possible for flink-table-planning. flink-table-api will be very challenging (if not impossible). So, before porting more Scala code to Java, we should reach consensus about our plans. There should be a thread on the dev mailing list that started exactly this discussion.

Thanks, Fabian

@bowenli86
Copy link
Member Author

bowenli86 commented Nov 5, 2018

Thanks, @fhueske. Sorry that the description might be a bit imprecise. I updated descriptions in both the PR and the JIRA ticket. You're right, it's not all code, here I'm mainly referring to external catalog related code, and @twalthr and I have discussed it in FLINK-10689. Let me explain here:

Background is that we have two parallel efforts going on right now - FLINK-10686, driven by Timo, includes moving external catalogs APIs from flink-table to flink-table-common, from Scala to Java; FLINK-10744 I'm working on right now to integrate Flink with Hive and enhance external catalog functionality.

As discussed with @twalthr in FLINK-10689, we'd better parallelize these efforts while introducing minimal overhead for integrating them later. An agreed way is to writing new code/feature related to external catalogs/hive in Java in flink-table first then move to other module like flink-table-common, this way we can minimize migration efforts. If existing classes are modified for a feature we can start migrating them to Java in a separate commit first and then perform the actual feature changes, and migrated classes can be placed in flink-table/src/main/java until we find a better module structure.

Does my explanation here relieve your concerns?

@fhueske
Copy link
Contributor

fhueske commented Nov 6, 2018

Hi @bowenli86, sure no worries.

I'm also in favor of moving the code base towards Java, but as I said I don't like a fragmented code base where tightly connected classes are in mixed languages. I just wanted to raise awareness that we don't want to implement all new (or heavily modified) classes in Java. Some parts of the flink-table code base (esp. most of the existing functionality) should remain implemented in Scala for now until we agree on a coordinated effort to move towards Java. For certain parts like the ones in this PR, Java is fine.

Copy link
Contributor

@twalthr twalthr left a comment

Choose a reason for hiding this comment

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

@bowenli86 I think it makes no sense to merge this PR, given that we will have a new catalog interface quite soon. What do you think?

@bowenli86 bowenli86 closed this Jan 21, 2019
@bowenli86
Copy link
Member Author

closing this PR in favor of new plan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants