[REEF-2021]In AzureBatch Runtime, Enable REEF .NET Client Communication to Driver #1468

homezcx · 2018-05-29T18:43:47Z

This approach enables REEF.NET Client-Driver communication by using InBoundNATPool features provided by Azure Batch Pool. More Info: https://docs.microsoft.com/en-us/rest/api/batchservice/pool/add#inboundnatpool

JIRA:
REEF-2021

Pull Request:
This closes #1468

homezcx · 2018-05-29T18:51:50Z

@tcondie This is the approach using Azure Batch InboundNATPool to enable the communication. More Info in Jira.

homezcx · 2018-05-30T23:02:23Z

Rebased master branch csproj file change to resolve conflicts.

homezcx · 2018-06-04T17:33:59Z

Ping if someone can review. Thanks.

singlis

Hi, I posted comments and suggestions.

singlis · 2018-06-01T14:53:05Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+using System.Linq;
+using Microsoft.Azure.Batch;
+using Microsoft.Azure.Batch.Common;
+#if REEF_DOTNET_BUILD


This #if can be removed now - it was used to support building both old and new projects. Ill create a JIRA to go through the code and remove as well.

singlis · 2018-06-01T14:59:28Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchDotNetClient.cs

@@ -45,6 +47,12 @@ public sealed class AzureBatchDotNetClient : IREEFClient
        private readonly AzureBatchService _batchService;
        private readonly JobJarMaker _jobJarMaker;
        private readonly AzureBatchFileNames _azbatchFileNames;
+        private readonly string _azureBatchAccountName;


Hi @homezcx
This set of data (batch account name, pool id, etc) gets repeated. This is currently defined in the AzureBatchClientConfiguration -- can that configuration be re-used at all in these cases as a structure for managing this data?

Definitely we can. I am kind not sure which one is better practice. Having a wrapper only contains getters with no logic, or just like this. Since we are using injection here, no pain to call new Instance(param1, param2, .....)

Let's wait for another vote maybe?

Discussed offline. I will fix this by implementing helper methods in AzureBatchService class which holds the credentials. So we don't need pass them around.

singlis · 2018-06-01T15:43:24Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+            }
+            catch (BatchException e)
+            {
+                throw new InvalidOperationException("driver http endpoint file is not ready.", e);


Is this really an invalid operation or is it a timing issue? Like maybe we checked too soon for the driver file? If its a timing issue, would waiting a retry help?

It is a timing issue and will be retried according to line 66-67. To trigger the retry policy, an exception must be thrown. Any return value including "null" will be considered as successful. Retrhow the exception for logging purpose.

Thanks for the clarification!

singlis · 2018-06-01T15:44:35Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+            else
+            {
+                LOGGER.Log(Level.Warning, "unable to get driver http endpoint. The format in remote file is not correct.");
+                return null;


This I feel like should throw because an empty file, which should not happen, is not recoverable.

You are right. It is not recoverable but we need this to warn people in future if they changed file DriverHttpEndpoint.txt.

Would it be worth adding comments that this is called from the RetryPolicy? Like a comment specifying we are returning null to exit the RetryPolicy.

I will add it.

singlis · 2018-06-01T15:46:21Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+            }
+
+            //// Get port
+            string[] driverIpAndPorts = driverHost.Split(':');


I would combine this with the code above since its all related to retrieving the ip and port from the string. Then throw invalid operation if that fails to parse.

I think it is good to separate recoverable exception (by throwing InvalidOperationException) and unrecoverable exception (by returning null).

ok - makes sense, I wasn't aware of having to return null when I reviewed.

singlis · 2018-06-01T15:51:13Z

lang/cs/Org.Apache.REEF.Client/Common/JobSubmissionResult.cs

-            get { return _driverUrl; }
+            get
+            {
+                if (_driverUrl != null)


minor nit - this could be simplified to if (_driverUrl == null){} return _driverUrl;

singlis · 2018-06-01T16:11:24Z

lang/cs/Org.Apache.REEF.Client/Common/JobSubmissionResult.cs

            while (status.IsActive())
            {
+                // Add sleep in while loop, whose value alligns with default heart beat interval.
+                Task.Delay(TimeSpan.FromSeconds(DriverStatusIntervalInSecond)).GetAwaiter().GetResult();
+                LOGGER.Log(Level.Info, "DriverStatus is " + status);


The task delay is an empty task right? Why not use Thread.Sleep()?

Had offline discussion with @singlis . The idea was that I thought using Task.Delay will hold the execution without locking the thread. But actually there is no difference with calling Thread.Sleep() and Task.Delay().GetAwaiter().GetResult().

Changed it back to Thread.Sleep()

singlis · 2018-06-01T16:25:07Z

lang/cs/Org.Apache.REEF.Client/YARN/RESTClient/HttpClientRetryHandler.cs

@@ -19,6 +19,7 @@
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
+using Org.Apache.REEF.Client.Common;


Is this using needed?

Yes. AllErrorsTransientStrategy is moved to Common, as it was not designed for YARN.

singlis · 2018-06-01T16:27:09Z

lang/cs/Org.Apache.REEF.Examples.HelloREEF/HelloREEF.cs

@@ -123,6 +124,11 @@ private static IConfiguration GetRuntimeConfiguration(string name)
                        .Set(AzureBatchRuntimeClientConfiguration.AzureStorageAccountKey, @"##########################################")
                        .Set(AzureBatchRuntimeClientConfiguration.AzureStorageAccountName, @"############")
                        .Set(AzureBatchRuntimeClientConfiguration.AzureStorageContainerName, @"###########")
+                        //// Extend default retry interval in Azure Batch
+                        .Set(AzureBatchRuntimeClientConfiguration.DriverHTTPConnectionRetryInterval, "20000")


What about driver connection attempts?

Default values works for our needs. So in this example we don't need to alter the value. Also, when using List, it gives a cannot convert error at compiling.

singlis · 2018-06-01T16:33:59Z

...idge-client/src/main/java/org/apache/reef/bridge/client/AzureBatchBootstrapREEFLauncher.java

+      networkConfiguration = client.poolOperations().getPool(poolId).networkConfiguration();
+    } catch (IOException e) {
+      LOG.log(Level.WARNING, "Unable to setup Http Server with InBoundNATPool Port", e);
+      return null;


I would not return null here - this is requiring that the code calling this api is checking for null. Looking at the code above that references this function, it looks like it does not and would fail with a null ref. I think this code should throw and be captured appropriately so that you can identify what the real error is.

Same goes for other cases where null is returned in this function.

Nice catch. I fixed this by returning empty set.

In this case, Driver Http server will fall back to use RangeTcpPortProvider.

singlis · 2018-06-04T22:30:53Z

Thanks, this looks good to me. @markusweimer or @motus, can you please take a second look?

homezcx · 2018-06-11T22:34:48Z

@markusweimer or @motus Could you help review this PR as well?

motus · 2018-06-14T20:47:46Z

Sure, I will look at it now. Meanwhile, @homezcx, could you please resolve the conflicts? Thank you!

homezcx · 2018-06-14T21:45:03Z

@motus Thanks! I will work on resolving the conflicts.

…on to Driver This approach enables REEF.NET Client-Driver communication by using InBoundNATPool features provided by Azure Batch Pool. More Info: https://docs.microsoft.com/en-us/rest/api/batchservice/pool/add#inboundnatpool JIRA: [REEF-2021](https://issues.apache.org/jira/browse/REEF-2021) Pull Request: This closes #

folder. Process the bridge jar as other runtime to avoid IOException caused by duplicate copy of bridge jar to global folder.

1. Fix potential null pointer excpetion when there is no InboundNATPool set. 2. Code cleanup

AzureBatchService to perform Azure Batch operations.

Node operation is only suppoted using AzureBatch key.

homezcx · 2018-06-14T22:47:40Z

@motus Rebased and tested.

sharathmalladi · 2018-06-15T00:27:42Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/Parameters/AzureBatchPoolDriverPortsList.cs

+namespace Org.Apache.REEF.Client.AzureBatch.Parameters
+{
+    [NamedParameter(Documentation = "The Azure Batch Pool Driver Http Server Ports List")]
+    public sealed class AzureBatchPoolDriverPortsList : Name<IList<string>>


string [](start = 67, length = 6)

Encountered this when looking at my related changes. Why this is not IList instead?

From the example in TestListInjection.java, to bind a List , it requires to use List

In reply to: 195608449 [](ancestors = 195608449)

By the way, it seems binding List in ConfigurationModule is not supported.

In reply to: 195804156 [](ancestors = 195804156,195608449)

motus

Left a few nit picks

motus · 2018-06-15T22:19:54Z

...idge-client/src/main/java/org/apache/reef/bridge/client/AzureBatchBootstrapREEFLauncher.java

+      result.add(sequence.toString());
+    }
+    return result;
+  }


use final; preallocate the result array; the signature can be more generic. I would write something like

private static List<String> asStringList(final Collection<? extends CharSequence> list) { final List<String> result = new ArrayList<>(list.size()); for (final CharSequence sequence : list) { result.add(sequence.toString()); } return result; }

motus · 2018-06-15T22:21:06Z

...java/reef-wake/wake/src/main/java/org/apache/reef/wake/remote/ports/ListTcpPortProvider.java

+  @Inject
+  public ListTcpPortProvider(@Parameter(TcpPortList.class) final List<Integer> tcpPortList) {
+    this.tcpPortList = tcpPortList;
+    LOG.log(Level.FINE, "Instantiating " + this);


use string interpolation

motus · 2018-06-15T22:47:22Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+
+        protected override string GetDriverUrl(string filepath)
+        {
+            var policy = new RetryPolicy<AllErrorsTransientStrategy>(_numberOfRetries, TimeSpan.FromMilliseconds(_retryInterval));


policy can be a member variable, initialized in constructor. then you won't need _numberOfRetries and _retryInterval members

motus · 2018-06-15T22:51:17Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+                LOGGER.Log(Level.Warning, "unable to get driver http endpoint. The format in remote file is not correct.");
+                //// Returns null to exit retry policy since it is not recoverable.
+                return null;
+            }


As far as I understand, http endpoint file may not necessarily have a terminating \n. I would rather write

string driverHost = httpEndPointFile.ReadAsString().TrimEnd('\r', '\n', ' ');

etc.

It is \n accourding to
https://github.com/apache/reef/blob/master/lang/java/reef-bridge-java/src/main/java/org/apache/reef/javabridge/generic/JobDriver.java#L193

But it is no harm to make the change.

motus · 2018-06-16T00:10:12Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+                LOGGER.Log(Level.Warning, "unable to get driver http endpoint port. The format in remote file is not correct.");
+                //// Returns null to exit retry policy since it is not recoverable.
+                return null;
+            }


I think it would be better to parse the port as int right away, i.e. do something like

string[] driverIpAndPorts = driverHost.Split(':'); if (driverIpAndPorts.Length <= 1 || !int.TryParse(driverIpAndPorts[1], out int backendPort)) { LOGGER.Log(Level.Warning, "Unable to get driver http endpoint port from: {0}", driverHost); return null; } // ... InboundEndpoint endpoint = inboundEndpoints.FirstOrDefault(s => s.BackendPort == backendPort);

motus · 2018-06-16T00:18:05Z

lang/cs/Org.Apache.REEF.Client/AzureBatch/AzureBatchJobSubmissionResult.cs

+            }
+
+            return "http://" + publicIp + ':' + frontEndPort + '/';
+        }


By the way, who writes the content of the httpEndPointFile? If it is us, can we write a proper URL to it instead, so that the string can be parsed with just

Uri driverUrl = new Uri(httpEndPointFile.ReadAsString());

We can also return a Uri object instead of a string from that method

It is JavaBridge writes the HttpEndPointFile. See:
https://github.com/apache/reef/blob/master/lang/java/reef-bridge-java/src/main/java/org/apache/reef/javabridge/generic/JobDriver.java#L193

The string format ip + port in this file is used by other runtime as well, YARN, local. We can change it but it seems to be out of scope of this PR. Shall we make it separate ticket?

motus · 2018-06-16T00:22:41Z

lang/cs/Org.Apache.REEF.Client/Common/JobSubmissionResult.cs

            while (status.IsActive())
            {
+                // Add sleep in while loop, whose value alligns with default heart beat interval.
+                Thread.Sleep(DriverStatusIntervalInMilliSecond);
+                LOGGER.Log(Level.Info, "DriverStatus is " + status);


use string interpolation

motus · 2018-06-16T00:25:06Z

lang/cs/Org.Apache.REEF.Client/Common/JobSubmissionResult.cs

@@ -85,7 +94,15 @@ internal JobSubmissionResult(IREEFClient reefClient, string filePath, int number
        /// </summary>
        public string DriverUrl


Same as above: maybe return Uri instead of a string URL?

Same as above: The current string format is "10.0.0.4:1234". it requires to change the string in HttpEndPointFile to a valid URI format, which could affect other runtimes. See if we want to create another ticket for it.

motus

Looks good! Will test locally and merge

motus · 2018-06-18T21:03:31Z

lang/cs/Org.Apache.REEF.Client/Common/JobSubmissionResult.cs


                try
                {
                    status = FetchDriverStatus();
                }
-                catch (WebException e)
+                catch (WebException)
                {
                    // If we no longer can reach the Driver, it must have exited.


I would still log the exception here, say

LOGGER.Log(Level.Warning, "Driver unreacheable. Exiting now.", e);

homezcx closed this May 30, 2018

homezcx deleted the features/inboundNatPool branch May 30, 2018 22:29

homezcx restored the features/inboundNatPool branch May 30, 2018 22:29

homezcx reopened this May 30, 2018

singlis reviewed Jun 4, 2018

View reviewed changes

singlis approved these changes Jun 11, 2018

View reviewed changes

chzhams added 7 commits June 14, 2018 14:52

Fix style violation in AzureBatchBootstrapREEFLauncher

07a59e9

Bridge jar is already added to global

03f3c02

folder. Process the bridge jar as other runtime to avoid IOException caused by duplicate copy of bridge jar to global folder.

Addrees review comments:

c591eb4

1. Fix potential null pointer excpetion when there is no InboundNATPool set. 2. Code cleanup

Refactor AzureBatchDotNetClient, AzureBatchJobSubmissionResult to use

9888249

AzureBatchService to perform Azure Batch operations.

Remove port check in AzureBatchBootstrapREEFLauncher.

492acaf

Node operation is only suppoted using AzureBatch key.

Fix BindException to TcpPortList after rebasing master

f10eab4

AzureBatchBootstrapREEFLauncher style violation fix

fd1dbe5

sharathmalladi reviewed Jun 15, 2018

View reviewed changes

motus suggested changes Jun 16, 2018

View reviewed changes

A few nit picks fix.

c3b1927

motus approved these changes Jun 18, 2018

View reviewed changes

Add warning when client is not able to reach driver

6070149

motus approved these changes Jun 19, 2018

View reviewed changes

motus merged commit b96c045 into apache:master Jun 19, 2018

		@@ -85,7 +94,15 @@ internal JobSubmissionResult(IREEFClient reefClient, string filePath, int number
		/// </summary>
		public string DriverUrl

[REEF-2021]In AzureBatch Runtime, Enable REEF .NET Client Communication to Driver #1468

[REEF-2021]In AzureBatch Runtime, Enable REEF .NET Client Communication to Driver #1468

Conversation

homezcx commented May 29, 2018 • edited

homezcx commented May 29, 2018

homezcx commented May 30, 2018

homezcx commented Jun 4, 2018

singlis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homezcx Jun 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homezcx Jun 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singlis commented Jun 4, 2018 • edited

homezcx commented Jun 11, 2018

motus commented Jun 14, 2018

homezcx commented Jun 14, 2018

homezcx commented Jun 14, 2018

Choose a reason for hiding this comment

homezcx Jun 15, 2018 • edited

Choose a reason for hiding this comment

homezcx Jun 15, 2018 • edited

Choose a reason for hiding this comment

motus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homezcx commented May 29, 2018 •

edited

homezcx Jun 4, 2018 •

edited

homezcx Jun 4, 2018 •

edited

singlis commented Jun 4, 2018 •

edited

homezcx Jun 15, 2018 •

edited

homezcx Jun 15, 2018 •

edited