[REEF-2025] A new module containing the new Java bridge

[tcondie]

This Jira introduces a new Java bridge for Drivers implemented in alternative languages. It provides the following artifacts (note: client driver refers to the application driver implemented in an alternative
programming language):

1. A generic framework for passing information between the Java driver and the client driver.
2. A gRPC based implementation of the bridge that passes information via protocol buffers over gRPC.
3. Protocol buffer definitions for all information that flows between the Java driver and the client driver.
4. A Java implementation of the driver client that can be used for developing unit tests and serve as a template for implementing a driver client (say in C#).
5. Test cases to fail based unit tests that cover the Java bridge and client.

Pull Request:
Closes #

[tcondie]

@motus ping
@markusweimer FYI: completed Java side of the new bridge.

[motus]

There are quite a few tests failing currently on my PC and on CI servers:

Failed tests:
  FailBridgeDriverTest.testDriverCompleted:139 expected:<COMPLETED> but was:<FAILED(org.apache.reef.tests.library.exceptions.DriverSideFailure: Event out of sequence: org.apache.reef.runtime.common.driver.task.TaskMessageImpl)>
  FailBridgeDriverTest.testFailDriverActiveContext:95->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverAlarm:125->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverAllocatedEvaluator:90->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverCompletedEvaluator:120->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverCompletedTask:115->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverConstructor:80->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverRunningTask:100->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverStart:85->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverStop:130->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeDriverTest.testFailDriverSuspendedTask:110->failOn:69 expected:<FAILED> but was:<FORCE_CLOSED>
  FailBridgeDriverTest.testFailDriverTaskMessage:105->failOn:69 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeTaskTest.testFailTask:63->failOn:54 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeTaskTest.testFailTaskCall:68->failOn:54 Unexpected error: java.lang.IllegalStateException: Unable to start driver client

  FailBridgeTaskTest.testFailTaskClose:93->failOn:54 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeTaskTest.testFailTaskMsg:73->failOn:54 expected:<FAILED> but was:<FORCE_CLOSED>
  FailBridgeTaskTest.testFailTaskStart:83->failOn:54 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailBridgeTaskTest.testFailTaskStop:88->failOn:54 expected:<FAILED> but was:<FORCE_CLOSED>
  FailBridgeTaskTest.testFailTaskSuspend:78->failOn:54 Unexpected error: java.lang.IllegalStateException: Unable to start driver client
  FailDriverTest.testDriverCompleted:131 expected:<COMPLETED> but was:<FORCE_CLOSED>
  FailDriverTest.testFailDriverCompletedEvaluator:112->failOn:64 Unexpected error: org.apache.reef.tests.library.exceptions.DriverSideFailure: Event out of sequence: org.apache.reef.runtime.common.driver.task.TaskMessageImpl
  FailDriverTest.testFailDriverConstructor:72->failOn:64 expected:<FAILED> but was:<FORCE_CLOSED>
  FailDriverTest.testFailDriverSuspendedTask:102->failOn:64 expected:<FAILED> but was:<FORCE_CLOSED>
  FailTaskTest.testFailTaskMsg:71->failOn:52 expected:<FAILED> but was:<FORCE_CLOSED>
  FailTaskTest.testFailTaskStop:86->failOn:52 expected:<FAILED> but was:<FORCE_CLOSED>

I will keep reviewing the code, but please feel free to push the fixes to this PR

[motus]

There is a file being generated during the build: lang/java/reef-bridge-proto-java/dependency-reduced-pom.xml. We should probably add it to .gitignore

[motus]

Left a few nit picks, will continue tomorrow

[motus]

We already have ExceptionInfo as part of the ContextInfo - do we need them both? is there a way to avoid such duplication?

[tcondie]

I believe both are warranted due to the context of the exception being specific to a context or a task. More specifically, when I create a FailedTask, I'd rather not initialize its exception from the exception in the ContextInfo.

[motus]

We need to be consistent in naming - i.e. stick to either camel case or underscores/snake_case for all IDs. I guess underscores are standard for gRPC?

[motus]

Also, this particular field should probably be failed_contexts_ids

[tcondie]

agreed.

[motus]

DriverClientConfiguration.runtime is an oneof field, and has a predefined set of runtimes; that is, we already have a hardcoded set of runtimes at the client. Should we use a fixed enum for runtimes here as well?

[tcondie]

makes sense.

[tcondie]

As stated, this makes sense to me. However, it creates complication when mapping the protocol buffer enum to the appropriate string in Java and C#. Ideally, this change would also include changing the internal java and C# types for runtime name to enums as well, so that the translation can be coded more robustly i.e., a compiler error is generated if the enum name changes on either end, or if a new runtime name is introduced (compiler warning).

Proposal: How about we create a Jira that chances the runtime name in all scenarios protocol buffers, C# and Java types? I suggest this because changing the runtime name type from strings to enums in Java and C# is not be relevant to this PR, which is dealing with the current codebase and all its design flaws :)

@markusweimer who might want to weigh in on this.

[motus]

We already have a predefined set of runtimes in ClientProtocol.proto - should we restrict our values here, too - e.g. using the enum?

[tcondie]

makes sense.

[tcondie]

see above reply to this.

[motus]

naming conventions - maybe use failed_task_id instead?

[tcondie]

yup, I'll go back and clean these up.

[motus]

We have raw payload fields here and in the TaskMessageInfo. Should we wrap it into some more abstract message - say, TaskMessage (with a payload field inside), and use it in both TaskMessageInfo and ClientMessageInfo? (or use TaskMessage instead of ClientMessageInfo?)

[tcondie]

I'm not sure TaskMessage makes since in the context of a ClientMessage. I'm not sure I understand the problem with using raw bytes payload.

[motus]

Same as above - use TaskMessage instead of raw bytes message?

[tcondie]

Not clear on why using raw bytes is a problem...

[motus]

instead of catch and throw here, just add IOException to throws clause in main()

[tcondie]

makes sense.

[motus]

no need to call .toString() explicitly; also, use string interpolation for logging, i.e.

LOG.log(Level.INFO, "Status: {0}", status);

[tcondie]

indeed.

[motus]

final

[motus]

Few more nit picks. Will continue tomorrow!

[motus]

No need to call Tang.Factory.getTang() again - we already have a global TANG object

[motus]

Here and everywhere in the file: use the global TANG object

[motus]

BindException is actually never thrown

[motus]

Also, I think we can throw InjectionException out of launch() without wrapping it into RuntimeException

[motus]

use string interpolation ({0})

[motus]

Map.remove() returns null if the key is not found. That is, we can do without .containsKey() call:

final ClientAlarm clientAlarm = this.alarmMap.remove(alarmId);
if (clientAlarm != null) {
  clientAlarm.run();
// ...

[motus]

same as above

[motus]

Looks like a common pattern. Should we add a helper function for it? like,

public static ExceptionInfo exceptionInfo(final Throwable ex) // ...

[motus]

final

[motus]

Should we make gRPC timeout configurable?

[motus]

add a string description, e.g.

throw new RuntimeException("Cannor register DriverClient", e);

[motus]

few more nitpicks - will continue tomorrow

[motus]

makes sense

[motus]

does it have to be synchronized at this point?

[motus]

BindException is actually never thrown

[motus]

indent the two lines above for readability

[motus]

final

[motus]

Same as above:

final Optional<ActiveContext> parent = Optional.<ActiveContext>ofNullable(
    this.activeContextBridgeMap.get(context.getParentId().get()));

[motus]

I wonder why DefaultExceptionCodec does not accept null and byte[0] input, like this:

@Override
public Optional<Throwable> fromBytes(final byte[] bytes) {
  try {
    if (bytes != null && bytes.length > 0) {
      return Optional.of((Throwable) SerializationUtils.deserialize(bytes));
    }
  } catch (final SerializationException | IllegalArgumentException e) {
    LOG.log(Level.WARNING, "Unable to deserialize a Throwable.", e);
  }
  return Optional.empty();
}

That would allow us to write

final Optional<Throwable> reason =
    this.exceptionCodec.fromBytes(request.getException().getData().toByteArray());

[motus]

Can simplify and reduce the number of trips to the map, e.g.

ActiveContextBridge context = null;
if (request.hasContext()) {
  final String contextId = request.getContext().getContextId();
  context = this.activeContextBridgeMap.get(contextId);
  if (context == null) {
    context = toActiveContext(request.getContext());
    this.activeContextBridgeMap.put(contextId, context);
  }
}

and then pass Optional.<ActiveContext>ofNullable(context) to constructor of the FailedTask

[motus]

one less trip to the map:

final ContextInfo contextInfo = request.getContext();
final String contextId = contextInfo.getContextId();

ActiveContextBridge context = this.activeContextBridgeMap.get(contextId);
if (context == null) {
  context = toActiveContext(contextInfo);
  this.activeContextBridgeMap.put(contextId, context);
}

[motus]

same as above re: number of trips to the map

[motus]

Done with the DriverClientSource. Will post more comments soon

[motus]

Looks like a common task - maybe have a utility function like

public static byte[] toByteArray(final ByteString bs) {
  return bs == null || bs.isEmpty() ? null : bs.toByteArray();
}

so we can write

GRPCUtils.toByteArray(request.getResult())

(and, maybe, also have versions to handle/produce Optional data?)

[motus]

I've just realized that we can have a small wrapper to make sure StreamObserver always gets closed:

public static class ObserverCleanup<T> implements AutoCloseable {

  private final StreamObserver<T> observer;
  private final T nextValue;

  public static <V> ObserverCleanup<V> of(
      final StreamObserver<V> observer) {
    return of(observer, null);
  }

  public static <V> ObserverCleanup<V> of(
      final StreamObserver<V> observer, final V nextValue) {
    return new ObserverCleanup<>(observer, nextValue);
  }

  private ObserverCleanup(final StreamObserver<T> observer, final T nextValue) {
    this.observer = observer;
    this.nextValue = nextValue;
  }

  @Override
  public void close() {
    this.observer.onNext(this.nextValue);
    this.observer.onCompleted();
  }
}

then our try block will look like this:

try (final ObserverCleanup _cleanup = ObserverCleanup.of(responseObserver)) {
  // ...
}

[motus]

use string interpolation

[motus]

use {0}

[motus]

or,

new EvaluatorException(request.getFailure() == null ?
    "restart failed" : request.getFailure().getMessage())

[motus]

fix the indentation

[motus]

We can probably have a special method for strings, say Optional.ofString()

[motus]

In fact, we do so many similar lookups in activeContextBridgeMap, that we can factor out the whole process into a helper method, e.g.

private ActiveContextBridge addContextIfMissing(final ContextInfo contextInfo) {
  final String contextId = contextInfo.getContextId();
  ActiveContextBridge context = this.activeContextBridgeMap.get(contextId);
  if (context == null) {
    context = toActiveContext(contextInfo);
    this.activeContextBridgeMap.put(contextId, context);
  }
  return context;
}

Then, with all other helpers mentioned earlier, our method would become just

@Override
public void completedTaskHandler(
    final TaskInfo request, final StreamObserver<Void> responseObserver) {
  final ActiveContextBridge context = this.addContextIfMissing(request.getContext());
  LOG.log(Level.INFO, "Completed task id {0}", request.getTaskId());
  try (final ObserverCleanup _cleanup = ObserverCleanup.of(responseObserver)) {
    this.clientDriverDispatcher.get().dispatch(new CompletedTaskBridge(
        request.getTaskId(), context, toByteArray(request.getResult())));
  }
}

[motus]

Few more nit picks and suggestions. To be continued!

[motus]

no need for boxing - use int

[motus]

All three task operations are very similar - maybe it makes sense to factor out the common part into a helper function? say,

private void taskOp(final RunningTaskRequest.Operation op, final String taskId, final byte[] message) {
  final RunningTaskRequest.Builder request = RunningTaskRequest.newBuilder().setTaskId(taskId).setOperation(op);
  if (message != null && message.length > 0) {
    request.setMessage(ByteString.copyFrom(message));
  }
  this.serviceStub.runningTaskOp(request.build());
}

@Override
public void onTaskClose(final String taskId, final Optional<byte[]> message) {
  this.taskOp(RunningTaskRequest.Operation.CLOSE, taskId, message.orElse(null));
}

@Override
public void onTaskMessage(final String taskId, final byte[] message) {
  this.taskOp(RunningTaskRequest.Operation.SEND_MESSAGE, taskId, message);
}

@Override
public void onSuspendTask(final String taskId, final Optional<byte[]> message) {
  this.taskOp(RunningTaskRequest.Operation.SUSPEND, taskId, message.orElse(null));
}

?

[motus]

we can throw InjectionException from launch() - it is being handled in every invocation context anyway. Then we don't need try/catch/fatal/LOG here at all

[motus]

add space before {

[motus]

same as with LocalLauncher: throw InjectionException out of the method and remove all error handling, including fatal()

[motus]

use string interpolation, remove .toString() call

[motus]

two lines above can be just

LOG.log(Level.SEVERE, "Job completed with error", status.getError().get());

this will print the exception and the stack trace to the log

[motus]

let InjectionException fly out of the launch() method.

[motus]

few more nit picks

[motus]

We don't need that method anymore

[motus]

Once we remove fatal() method, we don't need the logger, either.

[motus]

oops, replace + with ,

[motus]

here's another chunk of comments. stay tuned! 😄

[motus]

There are three parameters and only two interpolation placeholders. I guess we should have {2} at the end?

[motus]

final

[motus]

use string interpolation

[motus]

can use int instead of a boxed value

[motus]

no need for explicit else block

[motus]

same as above

[tcondie]

This represents an exception thrown from an alternative language that we cannot decode. GRPCUtils.createExceptionInfo deals with java only exceptions.

[motus]

same. we have to grep the code for all places where we still use ExceptionInfo.newBuilder() instead of .createExceptionInfo()

[motus]

use

.setParentId(task.getActiveContext().get().getParentId().orElse(""))

[motus]

use GRPCUtils.createExceptionInfo()

[motus]

use .orElse("")

[motus]

few more nit picks before the weekend 😄

[motus]

looks like pretty common operation. should we have a method like

private static ContextInfo toContextInfo(final ContextBase context) {
  return toContextInfo(context, null);
}

private static ContextInfo toContextInfo(final ContextBase context, final ExceptionInfo error) {
  final ContextInfo.Builder builder = ContextInfo.newBuilder()
      .setContextId(context.getId())
      .setEvaluatorId(context.getEvaluatorId())
      .setParentId(context.getParentId().orElse(""))
      .setEvaluatorDescriptorInfo(toEvaluatorDescriptorInfo(
          context.getEvaluatorDescriptor()));
  if (error != null) {
    builder.setException(error);
  }
  return builder.build();
}

then we can write

@Override
public synchronized void activeContextHandler(final ActiveContext context) {
  this.activeContextMap.put(context.getId(), context);
  this.clientStub.activeContextHandler(toContextInfo(context));
}

and

@Override
public synchronized void failedContextHandler(final FailedContext context) {
  final ContextInfo contextInfo;
  if (context.getReason().isPresent()) {
    contextInfo = toContextInfo(context,
      GRPCUtils.createExceptionInfo(this.exceptionCodec, context.getReason().get()));
  } else if (context.getData().isPresent()) {
    contextInfo = toContextInfo(context, ExceptionInfo.newBuilder()
        .setName(context.toString())
        .setMessage(context.getDescription().orElse(
            context.getMessage() != null ? context.getMessage() : ""))
        .setData(ByteString.copyFrom(context.getData().get()))
        .build());
  } else {
    contextInfo = toContextInfo(context,
        GRPCUtils.createExceptionInfo(this.exceptionCodec, context.asError()));
  }
  this.activeContextMap.remove(context.getId());
  this.clientStub.failedContextHandler(contextInfo);
}

and so on? (same, BTW, applies to many other gRPC messages that we have)

[motus]

make it static

[motus]

use string interpolation

[motus]

use string interpolation, remove explicit .toString() call

[motus]

here and in other methods below: use

try (final ObserverCleanup _cleanup = ObserverCleanup.of(responseObserver)) {
  // ...
}

[motus]

use string interpolation (here and everywhere)

[motus]

one less trip to the map:

final String contextId = request.getContextId();
final ActiveContext context = GRPCDriverService.this.activeContextMap.get(contextId);
if (context == null) {
  LOG.log(Level.SEVERE, "Context does not exist with id {0}", contextId);
  responseObserver.onError(Status.INTERNAL
      .withDescription("Context does not exist with id " + contextId)
      .asRuntimeException());
  return;
}
// ...

[motus]

do

switch (request.getOperationCase()) {
  case CLOSE_CONTEXT:
    // ...
}

[motus]

use string interpolation

[motus]

also,

try (final ObserverCleanup _cleanup = ObserverCleanup.of(responseObserver)) {
  // ...
}

[motus]

Final round of nit picks - finally made it to the end of the PR! 😄

[motus]

No need to check for .getLocalLibrariesCount() - the call

localLibraries.addAll(driverConfiguration.getLocalLibrariesList());

will work correctly (i.e. be a no-op) even when the list is empty

[motus]

can be final

[motus]

maybe just

private static final ClientProtocol.DriverClientConfiguration CLIENT_CONFIG =
    ClientProtocol.DriverClientConfiguration.newBuilder()
        .setJobid("HelloREEF")
        .setEnableHttpDriver(false)
        .setOperatingSystem(ClientProtocol.DriverClientConfiguration.OS.LINUX)
        .setAzbatchRuntime(ClientProtocol.AzureBatchRuntimeParameters.newBuilder().build())
        .addGlobalLibraries(EnvironmentUtils.getClassLocation(HelloDriver.class))
        .build();

[motus]

can remove the if statement: there is no need to check for .getLocalLibrariesCount() - the call localLibraries.addAll() will work correctly even when the input list is empty.

[motus]

final

[motus]

or,

.setClassPath(StringUtils.join(classpathProvider.getDriverClasspath(),
    driverClientConfigurationProto.getOperatingSystem() ==
        ClientProtocol.DriverClientConfiguration.OS.WINDOWS ? ";" : ":"))

[motus]

there's already a static TANG object

[motus]

maybe,

final ClientProtocol.DriverClientConfiguration driverServiceConfiguration =
    ClientProtocol.DriverClientConfiguration.newBuilder(driverClientConfigurationProto)
        .setDriverClientLaunchCommand(cmd)
        .addLocalFiles(driverClientConfigurationFile.getAbsolutePath())
        .build();

return driverServiceConfigurationProvider.getDriverServiceConfiguration(driverServiceConfiguration);

[tcondie]

@motus Just finished with your final round of reviews. Thank you for helping improve the code quality! Please let me know if you have any further change requests. If not, then perhaps we can roll this into master, possibly after REEF-2024 if you like it.

[markusweimer]

I did a surface skim for issues with docs, APIs and project structure. Please have a look and generalize my comments to other parts of the PR as appropriate.

[markusweimer]

Please add documentation to the new message types introduced in this PR.

[markusweimer]

Can you please push all of these into the parent pom? Also, please add the dependencies to <dependencyManagement> in that parent POM and drop the versions in here.

[markusweimer]

For all new public types: Please decide whether they truly need to be public. If so, decide whether they create new APIs of REEF. If not, mark them @Private, not to introduce new support surface.

[markusweimer]

Please document all public types & members with JavaDoc

[markusweimer]

Does this launch the job or the driver?

[tcondie]

I'm thinking they are the same thing

[tcondie]

I see your point now. This is launching the REEF job, which is a DriverService + application DriverClient. I reuse the REEFLauncher to start the DriverService.Driver, which in turn will fire up the application DriverClient. Please let me know if you think things could be more clear.

[markusweimer]

Shouldn't it also fail here?

[markusweimer]

Consider marking this, and other legitimately public new APIs @Unstable to mark their status of development.

[markusweimer]

Do we need a copy of the Driver / Task? I'd like to see a re-use of existing examples to validate that existing REEF apps can be ported over to this new runtime.

[tcondie]

The driver adds a couple of extra event handlers, but I see your point and will target reef-examples/Driver/Task implementations.

[tcondie]

@motus @markusweimer FYI: finished merging with #1467. Let me know if you guys think this PR is ready to go. I'd like to start syncing with another branch containing the C# side of the bridge.

[motus]

@tcondie I will take another pass thorough the code now, and post my approval here.

@markusweimer, I think the sheer size of the change requires a second pair of eyeballs to review this PR before I merge it. WDYT?

[markusweimer]

@motus I did a surface scan for maintainability issues. Assuming they are addressed, I am fine with merging this.

[motus]

Thanks @markusweimer for looking at it. I am running the tests now and doing a final scan over the code, and will merge it soon afterwards.

[motus]

left a few more minor nit picks. about 50% through the final review

[motus]

maybe,

try (final Reader reader = new FileReader(args[0])) {
  JsonFormat.parser()
      .usingTypeRegistry(JsonFormat.TypeRegistry.getEmptyTypeRegistry())
      .merge(reader, driverClientConfigurationProtoBuilder);
}

(otherwise I am not sure about encoding from bytes to String)

[motus]

no need to print the class name - it is included in the log message anyway. I would just write something like

LOG.log(Level.SEVERE,
    "Expected a single command line argument with a file containing client protobuf driver configuration");

[motus]

we can actually remove some code duplication here. e.g. instead of having three functions get*DriverServiceLauncher(), we can write something like this:

private static LauncherStatus launch(
    final ClientProtocol.DriverClientConfiguration driverClientConfigurationProto)
    throws InjectionException {

  final ClientProtocol.DriverClientConfiguration.RuntimeCase runtime =
      driverClientConfigurationProto.getRuntimeCase();

  final Class<? extends IDriverLauncher> launcherClass;
  switch (runtime) {
    case YARN_RUNTIME:
      launcherClass = YarnLauncher.class;
      break;
    case LOCAL_RUNTIME:
      launcherClass = LocalLauncher.class;
      break;
    case AZBATCH_RUNTIME:
      launcherClass = AzureBatchLauncher.class;
      break;
    default:
      throw new RuntimeException("Unknown runtime: " + runtime);
  }

  final Configuration jobSubmissionClientConfig = TANG.newConfigurationBuilder()
      .bindImplementation(IDriverLauncher.class, launcherClass)
      .bindImplementation(IDriverServiceConfigurationProvider.class,
          GRPCDriverServiceConfigurationProvider.class)
      .build();

  final IDriverLauncher driverServiceLauncher =
      TANG.newInjector(jobSubmissionClientConfig).getInstance(launcherClass);

  return driverServiceLauncher.launch(driverClientConfigurationProto);
}

[motus]

Prefix I for interfaces is standard in C#, but not in Java. Our coding conventions are:

• Do not use one-letter prefixes; name interfaces as some general entities they represent, e.g. Failure, AlarmDispatchHandler
• Prefix abstract classes with Abstract, e.g. AbstractFailure
• Use single letters T, U, V for generic type parameters; in rare cases it is OK to give prefix T to such arguments, e.g. Map<TKey, TValue>

[motus]

remove extra blank line. run the code formatter to make sure there are no double blank lines, empty lines before closing }, trailing spaces etc.

[motus]

Few more comments regarding C#-style naming for Java interfaces

[motus]

Noticed few more C#-style namings in Java. I would call the interface DriverClientService, and the implementation DefaultDriverClientService or DriverClientServiceImpl

[motus]

same as above. use DriverServiceClient for the interface, and e.g. DefaultDriverServiceClient for the implementation

[motus]

same as above. I would call the interface DriverLauncher, but this name is already taken by another class at the client side. Ideally, we should've renamed the existing DriverLauncher into something more appropriate (say, ReefClient) and use the DriverLauncher name here. Unfortunately, renaming the existing files is not an option for this PR, so we have to come up with a better name for our interface. I really don't know what to suggest - maybe, BridgeDriverLauncher (reflecting the fact that its method takes a protobuf config)?

[motus]

Same as above. I would just call it DriverService

[motus]

same as above. remove the I from the name

[motus]

OK, I am done with my final round! I've left a few very minor comments. Unfortunately, now that we are on gitbox, I cannot fix these issues myself, rebase, and push everything to the master. Hence those petty comments. Once they are fixed, I'll be happy to test and merge this PR immediately.

[motus]

add an error message, e.g. "Getting command line on the client is not supported." or something

[motus]

remove extra blank line

[motus]

remove extra blank line

[motus]

remove the empty line

[motus]

remove the empty line

[motus]

remove empty line

[motus]

remove empty line

[motus]

remove empty line

[motus]

remove empty line

[motus]

remove empty line