subject:"\[GitHub\] \[flink\] tillrohrmann commented on a change in pull request #9609\: \[FLINK\-13750\]\[client\]\[coordination\] Separate HA services between client\-side and server\-side"

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320709782
 
 

 ##
 File path: 
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/HighAvailabilityServicesUtilsTest.java
 ##
 @@ -59,6 +59,22 @@ public void testCreateCustomHAServices() throws Exception {
assertSame(haServices, actualHaServices);
}
 
+   @Test
+   public void testCreateCustomClientHAServices() throws Exception {
+   Configuration config = new Configuration();
+
+   ClientHighAvailabilityServices clientHAServices = 
Mockito.mock(ClientHighAvailabilityServices.class);
 
 Review comment:
   I know that this test class already uses `Mockito`. But I would like to 
avoid adding more Mockito mocks. Instead we could have a very simple 
`TestingClientHighAvailabilityServices` implementation.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320708752
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/ClientHighAvailabilityServices.java
 ##
 @@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.highavailability;
+
+import org.apache.flink.runtime.leaderretrieval.LeaderRetrievalService;
+
+/**
+ * {@code ClientHighAvailabilityServices} provides services those are required
+ * in client-side. At the moment only web monitor leader retriever is required
+ * because all requests from client are received and propagated by web monitor.
+ */
+public interface ClientHighAvailabilityServices extends AutoCloseable {
+
+   /**
+* Get the leader retriever for the web monitor.
+*
+* @return the leader retriever for the web monitor.
+*/
+   LeaderRetrievalService getWebMonitorLeaderRetriever();
 
 Review comment:
   Technically speaking it should be the `RestEndpoint` because `WebMonitor` 
denotes for me more the web UI. It just happens to be the case that our 
`WebMonitor` runs on the same netty server as the cluster `RestEndpoint`. Hence 
I would be in favour of calling it `getRestEndpointLeaderRetriever` or 
`getClusterRestEndpointLeaderRetriever`.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320671873
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/HighAvailabilityServices.java
 ##
 @@ -98,6 +98,15 @@
 */
LeaderRetrievalService getJobManagerLeaderRetriever(JobID jobID, String 
defaultJobManagerAddress);
 
+   /**
+* This retriever should be no longer used in cluster side. The 
retriever to web monitor
+* is only required in client-side and we have a dedicated 
high-availability services
 
 Review comment:
   ```suggestion
 * is only required on the client-side and we have a dedicated 
high-availability services
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320669884
 
 

 ##
 File path: 
flink-clients/src/test/java/org/apache/flink/client/RemoteExecutorHostnameResolutionTest.java
 ##
 @@ -51,35 +50,34 @@ public static void check() {
 
@Test
public void testUnresolvableHostname1() throws Exception {
-
RemoteExecutor exec = new RemoteExecutor(nonExistingHostname, 
port);
+
try {
exec.executePlan(getProgram());
fail("This should fail with an 
ProgramInvocationException");
-   }
-   catch (UnknownHostException ignored) {
-   // that is what we want!
+   } catch (UnknownHostException ignored) {
+   // expected
}
}
 
@Test
public void testUnresolvableHostname2() throws Exception {
-
-   InetSocketAddress add = new 
InetSocketAddress(nonExistingHostname, port);
-   RemoteExecutor exec = new RemoteExecutor(add, new 
Configuration(),
-   Collections.emptyList(), 
Collections.emptyList());
+   RemoteExecutor exec = new RemoteExecutor(
+   new InetSocketAddress(nonExistingHostname, port),
+   new Configuration(),
+   Collections.emptyList(),
+   Collections.emptyList());
try {
exec.executePlan(getProgram());
fail("This should fail with an 
ProgramInvocationException");
-   }
-   catch (UnknownHostException ignored) {
+   } catch (UnknownHostException ignored) {
// that is what we want!
}
}
 
private static Plan getProgram() {
ExecutionEnvironment env = 
ExecutionEnvironment.getExecutionEnvironment();
-   env.fromElements(1, 2, 3).output(new 
DiscardingOutputFormat());
+   env.fromElements(1, 2, 3).output(new 
DiscardingOutputFormat<>());
 
 Review comment:
   The changes in this file seem to be unrelated. I'd suggest to keep them but 
in the future, let's try to avoid them.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320674447
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/zookeeper/ZKClientHAServices.java
 ##
 @@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.highavailability.zookeeper;
+
+import org.apache.flink.configuration.Configuration;
+import 
org.apache.flink.runtime.highavailability.ClientHighAvailabilityServices;
+import org.apache.flink.runtime.leaderretrieval.LeaderRetrievalService;
+import org.apache.flink.runtime.util.ZooKeeperUtils;
+
+import org.apache.curator.framework.CuratorFramework;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ZooKeeper based implementation for {@link ClientHighAvailabilityServices}.
+ */
+public class ZKClientHAServices implements ClientHighAvailabilityServices {
+
+   private static final String REST_SERVER_LEADER_PATH = 
"/rest_server_lock";
+
+   private final CuratorFramework client;
+   private final Configuration configuration;
+
+   public ZKClientHAServices(
+   @Nonnull CuratorFramework client,
+   @Nonnull Configuration configuration
+   ) {
 
 Review comment:
   nit: I think the dominating style is
   
   ```
   public A(
 B b) {
   }
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320675664
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorUtils.java
 ##
 @@ -142,6 +149,33 @@ private static File resolveFileLocation(String 
logFilePath) {
}
}
 
+   /**
+* Get address of web monitor from configuration.
+*
+* @param configuration Configuration contains those for WebMonitor.
+* @param resolution Whether to try address resolution of the given 
hostname or not.
+*   This allows to fail fast in case that the hostname 
cannot be resolved.
+* @return Address of WebMonitor.
+*/
+   public static String getWebMonitorAddress(
+   Configuration configuration,
+   HighAvailabilityServicesUtils.AddressResolution resolution
+   ) throws UnknownHostException {
 
 Review comment:
   I think this formatting style differs from the rest of the code in this file.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320674131
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/zookeeper/ZKClientHAServices.java
 ##
 @@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.highavailability.zookeeper;
+
+import org.apache.flink.configuration.Configuration;
+import 
org.apache.flink.runtime.highavailability.ClientHighAvailabilityServices;
+import org.apache.flink.runtime.leaderretrieval.LeaderRetrievalService;
+import org.apache.flink.runtime.util.ZooKeeperUtils;
+
+import org.apache.curator.framework.CuratorFramework;
+
+import javax.annotation.Nonnull;
+
+/**
+ * ZooKeeper based implementation for {@link ClientHighAvailabilityServices}.
+ */
+public class ZKClientHAServices implements ClientHighAvailabilityServices {
 
 Review comment:
   Should we keep it consistent with `ZooKeeperHaServices`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320676746
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorUtils.java
 ##
 @@ -142,6 +149,33 @@ private static File resolveFileLocation(String 
logFilePath) {
}
}
 
+   /**
+* Get address of web monitor from configuration.
+*
+* @param configuration Configuration contains those for WebMonitor.
+* @param resolution Whether to try address resolution of the given 
hostname or not.
+*   This allows to fail fast in case that the hostname 
cannot be resolved.
+* @return Address of WebMonitor.
+*/
+   public static String getWebMonitorAddress(
 
 Review comment:
   I'd rather put this method into the `HighAvailabilityServicesUtils` class.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320675766
 
 

 ##
 File path: 
flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java
 ##
 @@ -149,29 +153,16 @@ public RestClusterClient(Configuration config, T 
clusterId) throws Exception {
config,
null,
clusterId,
-   new ExponentialWaitStrategy(10L, 2000L),
-   null);
-   }
-
-   public RestClusterClient(
-   Configuration config,
-   T clusterId,
-   LeaderRetrievalService webMonitorRetrievalService) 
throws Exception {
-   this(
-   config,
-   null,
-   clusterId,
-   new ExponentialWaitStrategy(10L, 2000L),
-   webMonitorRetrievalService);
+   new ExponentialWaitStrategy(10L, 2000L));
}
 
@VisibleForTesting
RestClusterClient(
Configuration configuration,
@Nullable RestClient restClient,
T clusterId,
-   WaitStrategy waitStrategy,
-   @Nullable LeaderRetrievalService 
webMonitorRetrievalService) throws Exception {
+   WaitStrategy waitStrategy
+   ) throws Exception {
 
 Review comment:
   I think this formatting style differs from the rest of the code in this file.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320666723
 
 

 ##
 File path: 
flink-clients/src/main/java/org/apache/flink/client/program/MiniClusterClient.java
 ##
 @@ -157,15 +154,20 @@ public String stopWithSavepoint(JobID jobId, boolean 
advanceToEndOfEventTime, @N
return MiniClusterId.INSTANCE;
}
 
-   // ==
-   // Legacy methods
-   // ==
-
@Override
public String getWebInterfaceURL() {
return miniCluster.getRestAddress().toString();
}
 
+   @Override
+   public void shutDownCluster() {
+   try {
+   miniCluster.close();
+   } catch (Exception e) {
+   ExceptionUtils.rethrow(e);
+   }
+   }
 
 Review comment:
   Unrelated change


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320670888
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/ClientHighAvailabilityServices.java
 ##
 @@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.highavailability;
+
+import org.apache.flink.runtime.leaderretrieval.LeaderRetrievalService;
+
+/**
+ * {@code ClientHighAvailabilityServices} provides services those are required
+ * in client-side. At the moment only web monitor leader retriever is required
+ * because all requests from client are received and propagated by web monitor.
+ */
+public interface ClientHighAvailabilityServices extends AutoCloseable {
+
+   /**
+* Get the leader retriever for the web monitor.
+*
+* @return the leader retriever for the web monitor.
+*/
+   LeaderRetrievalService getWebMonitorLeaderRetriever();
 
 Review comment:
   Should we rename the leader retriever to `getRestEndpointLeaderRetriever`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320670131
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/ClientHighAvailabilityServices.java
 ##
 @@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.highavailability;
+
+import org.apache.flink.runtime.leaderretrieval.LeaderRetrievalService;
+
+/**
+ * {@code ClientHighAvailabilityServices} provides services those are required
+ * in client-side. At the moment only web monitor leader retriever is required
 
 Review comment:
   ```suggestion
* on client-side. At the moment only the REST endpoint leader retriever is 
required
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320671940
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/HighAvailabilityServices.java
 ##
 @@ -98,6 +98,15 @@
 */
LeaderRetrievalService getJobManagerLeaderRetriever(JobID jobID, String 
defaultJobManagerAddress);
 
+   /**
+* This retriever should be no longer used in cluster side. The 
retriever to web monitor
+* is only required in client-side and we have a dedicated 
high-availability services
+* for client, named {@link ClientHighAvailabilityServices}. See also 
FLINK-13750.
 
 Review comment:
   ```suggestion
 * for the client, named {@link ClientHighAvailabilityServices}. See 
also FLINK-13750.
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320670719
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/ClientHighAvailabilityServices.java
 ##
 @@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.highavailability;
+
+import org.apache.flink.runtime.leaderretrieval.LeaderRetrievalService;
+
+/**
+ * {@code ClientHighAvailabilityServices} provides services those are required
+ * in client-side. At the moment only web monitor leader retriever is required
+ * because all requests from client are received and propagated by web monitor.
 
 Review comment:
   ```suggestion
* because all communication between the client and cluster happens via the 
REST endpoint.
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320665694
 
 

 ##
 File path: 
flink-clients/src/main/java/org/apache/flink/client/program/MiniClusterClient.java
 ##
 @@ -51,14 +51,13 @@
private final MiniCluster miniCluster;
 
public MiniClusterClient(@Nonnull Configuration configuration, @Nonnull 
MiniCluster miniCluster) {
-   super(configuration, miniCluster.getHighAvailabilityServices(), 
true);
-
+   super(configuration);
this.miniCluster = miniCluster;
}
 
@Override
public void shutdown() throws Exception {
-   super.shutdown();
+   // no op
 
 Review comment:
   If there is a base implementation, then we don't need the no op override 
here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320671722
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/HighAvailabilityServices.java
 ##
 @@ -98,6 +98,15 @@
 */
LeaderRetrievalService getJobManagerLeaderRetriever(JobID jobID, String 
defaultJobManagerAddress);
 
+   /**
+* This retriever should be no longer used in cluster side. The 
retriever to web monitor
 
 Review comment:
   ```suggestion
 * This retriever should no longer be used on the cluster side. The web 
monitor retriever
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

2019-09-04 Thread GitBox

tillrohrmann commented on a change in pull request #9609: 
[FLINK-13750][client][coordination] Separate HA services between client-side 
and server-side
URL: https://github.com/apache/flink/pull/9609#discussion_r320665466
 
 

 ##
 File path: 
flink-clients/src/main/java/org/apache/flink/client/program/ClusterClient.java
 ##
 @@ -103,55 +94,21 @@
 * if that is not possible.
 *
 * @param flinkConfig The config used to obtain the job-manager's 
address, and used to configure the optimizer.
-*
-* @throws Exception we cannot create the high availability services
 */
-   public ClusterClient(Configuration flinkConfig) throws Exception {
-   this(
-   flinkConfig,
-   
HighAvailabilityServicesUtils.createHighAvailabilityServices(
-   flinkConfig,
-   Executors.directExecutor(),
-   
HighAvailabilityServicesUtils.AddressResolution.TRY_ADDRESS_RESOLUTION),
-   false);
-   }
-
-   /**
-* Creates a instance that submits the programs to the JobManager 
defined in the
-* configuration. This method will try to resolve the JobManager 
hostname and throw an exception
-* if that is not possible.
-*
-* @param flinkConfig The config used to obtain the job-manager's 
address, and used to configure the optimizer.
-* @param highAvailabilityServices HighAvailabilityServices to use for 
leader retrieval
-* @param sharedHaServices true if the HighAvailabilityServices are 
shared and must not be shut down
-*/
-   public ClusterClient(
-   Configuration flinkConfig,
-   HighAvailabilityServices highAvailabilityServices,
-   boolean sharedHaServices) {
+   public ClusterClient(Configuration flinkConfig) {
this.flinkConfig = Preconditions.checkNotNull(flinkConfig);
this.compiler = new Optimizer(new DataStatistics(), new 
DefaultCostEstimator(), flinkConfig);
-
this.timeout = AkkaUtils.getClientTimeout(flinkConfig);
-
-   this.highAvailabilityServices = 
Preconditions.checkNotNull(highAvailabilityServices);
-   this.sharedHaServices = sharedHaServices;
}
 
// 

//  Startup & Shutdown
// 

 
/**
-* Shuts down the client. This stops the internal actor system and 
actors.
+* Shuts down the client. This stops possible internal services.
 */
-   public void shutdown() throws Exception {
-   synchronized (this) {
-   if (!sharedHaServices && highAvailabilityServices != 
null) {
-   highAvailabilityServices.close();
-   }
-   }
-   }
+   public abstract void shutdown() throws Exception;
 
 Review comment:
   Could this have an empty implementation?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

[GitHub] [flink] tillrohrmann commented on a change in pull request #9609: [FLINK-13750][client][coordination] Separate HA services between client-side and server-side

17 matches

Site Navigation

Mail list logo

Footer information