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

[BEAM-3326] Remove SdkHarnessClientControlService #4761

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
3 participants
@tgroh
Member

tgroh commented Feb 27, 2018

The utility of this service is minimal, at best. The actual client
construction is a trivial call to SdkHarnessClient.create, and users
can just wrap the lower-level client with that call.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand:
    • What the pull request does
    • Why it does it
    • How it does it
    • Why this approach
  • Each commit in the pull request should have a meaningful subject line and body.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.
@tgroh

This comment has been minimized.

Member

tgroh commented Feb 27, 2018

@bsidhom

This comment has been minimized.

Contributor

bsidhom commented Feb 27, 2018

@axelmagn FYI

@lukecwik

This comment has been minimized.

Member

lukecwik commented Feb 27, 2018

@angoenka Might be interested in these changes since he has been working towards something like this for Dataflow.

public SdkHarnessClient getClient() {
try {
// Block until a client is available.
FnApiControlClient getClient = pendingClients.take();

This comment has been minimized.

@bsidhom

bsidhom Mar 1, 2018

Contributor

The fact that FnApiControlClientPoolService requires direct interaction with the underlying client queue makes direct work with the FnApiControlClientPoolService difficult to test, as I mentioned in #4751 . I think that this is a compelling enough reason to keep this interface until/if we change the FnApiControlClientPoolService interface.

@tgroh

This comment has been minimized.

Member

tgroh commented Mar 15, 2018

Assuming the changes in #4866 are accepted, this likely becomes a no-op including when run against flink, because it removes the need for the EnvironmentManager to directly access data or state services.

@tgroh

This comment has been minimized.

Member

tgroh commented Apr 2, 2018

@bsidhom Is your concern still true?

@bsidhom

This comment has been minimized.

Contributor

bsidhom commented Apr 2, 2018

No. We can go ahead and remove this now.

@bsidhom

bsidhom approved these changes Apr 2, 2018

@bsidhom

This comment has been minimized.

Contributor

bsidhom commented Apr 3, 2018

It looks like there are merge conflicts now. Is the plan still to remove this?

Remove SdkHarnessClientControlService
The utility of this service is minimal, at best. The actual client
construction is a trivial call to `SdkHarnessClient.create`, and users
can just wrap the lower-level client with that call.

@tgroh tgroh merged commit f5afe53 into apache:master Apr 19, 2018

2 of 3 checks passed

Jenkins: ./gradlew --info --continue --rerun-tasks --no-daemon :pythonPreCommit FAILURE
Details
Jenkins: ./gradlew --info --continue --rerun-tasks --no-daemon :goPreCommit SUCCESS
Details
Jenkins: ./gradlew :javaPreCommit SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment