Skip to content

Conversation

kl0u
Copy link
Contributor

@kl0u kl0u commented Oct 25, 2017

What is the purpose of the change

Make the Queryable State more usable by:

  1. Reducing the dependencies the client jar (used by the user) transitively brings.
  2. Putting the core and client jars in the opt/ folder. From there, to activate QS the user has to put the core jar in the lib/ folder before starting the cluster, and use the client jar as a dependency to his/her program.

Brief change log

Creating the core and client modules in the queryable state dir and moving classes around.
In addition, now the client module becomes a dependency of the flink-runtime.

Places the jars corresponding the aforementioned modules to the opt dir.

Verifying this change

This change is a simple restructuring of the queryable state module, no classes are introduced or deleted.

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

  • Dependencies (does it add or upgrade a dependency): yes, adds the flink-queryable-state-client dependency to flink-runtime.

Documentation

  • Does this pull request introduce a new feature? NO, but it needs documentation, which is PENDING.

R @aljoscha

Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

Very good changes? I had some nitpicks in the code and some more here. 😅

Does the client jar have to be in opt?

I think queryable-state-core should be called queryable-state-runtime or queryable-state-server to fit into existing Flink Terminology. For example, flink-core is this quite thin module that has mostly API classes and some stuff that is commonly used. What do you think?

*
* <p><b>THIS WAS COPIED FROM RUNTIME SO THAT WE AVOID THE DEPENDENCY.</b>
*/
public final class VoidNamespace {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be package private?

import java.util.concurrent.CompletableFuture;

/**
* Javadoc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing Javadoc.

@aljoscha
Copy link
Contributor

Btw, I'm assuming you also tried this with a cluster where you put the "core" jar in lib and then could query the state of a running job?

@kl0u kl0u force-pushed the qs-restructure branch 4 times, most recently from 633ad8f to f25ada7 Compare October 26, 2017 12:58
@aljoscha
Copy link
Contributor

LGTM with the name changes!

Excellent work. 👍

@kl0u
Copy link
Contributor Author

kl0u commented Oct 26, 2017

Thanks a lot for the review @aljoscha ! Waiting for travis and then merging.

@kl0u kl0u force-pushed the qs-restructure branch 3 times, most recently from b601359 to 9bfe6fd Compare October 26, 2017 16:10
kl0u added 2 commits October 26, 2017 18:57
The QS module is split into core and client. The core should
be put in the lib folder to enable queryable state, while the
client is the one that the user will program against. The
reason for the restructuring in mainly to remove the dependency
on the flink-runtime from the user's program.
Now the user can find the jars in the opt/ folder and
he can activate QS by putting the core jar in the lib/
folder and program against the client jar.
@asfgit asfgit merged commit 2fd8721 into apache:master Oct 26, 2017
@tillrohrmann
Copy link
Contributor

Sorry for joining the party late, but I don't think that it is a good idea to duplicate code by copying code from one module into the other, as happened for the FutureUtils for example. The proper solution would be to move the FutureUtils into flink-core such that they are accessible from all places. Doing differently will lead us to maintenance hell quite quickly.

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

Successfully merging this pull request may close these issues.

5 participants