restores highly available service

[keith-turner]

Restored highly available service in order to support multiple managers #6139 . This change should also avoid clients that connect to a non-primary manager from getting stuck. Made all thrift methods throw ThriftNotActiveServiceException, if the method does not declare this it will cause a server side exception. There was a single one way thrift method that could not throw this, so it would still cause server side exceptions if its called. Delaying advertising should prevent this in most cases.

[keith-turner]

Now that the compaction coordinator can throw ThriftNotActiveServiceException the compactors should retry if they see this exception. Looked at the compactors and it seems like they will retry as all of their calls use RetryableThriftCall.

[dlmarion]

I wonder if there is a way to automate a check that all Thrift RPC methods handled by the Manager throws the ThriftNotActiveServiceException

[keith-turner]

I wonder if there is a way to automate a check that all Thrift RPC methods handled by the Manager throws the ThriftNotActiveServiceException

Not sure this functionality has any tests. There is the server side aspect of throwing the exception, if we could automate test for that it would be nice. Then there is the client side code that retries when it sees this exception, that code is sprinkled all over the place. The server side code seems easier to test. For the client side code, maybe the best way to test is to run random walk tests while starting/stopping managers.

[keith-turner]

For the client side code, maybe if we refactored to use more common code on the client side then we could focus on testing that common code. I noticed the compactor code did not need to change because it uses common code for retry. That common code retries on TException though which may be too broad for the general case, like we probably should not retry on a thrift security exception.

[dlmarion]

I looked at trying to write a test for this, but could not figure it out quickly. Another option would be to fail at runtime on the server side, which should be caught during testing. To do this, we could include the following method in the ThriftProcessorTypes class, then call it for each interface passed to the getManagerTProcessor method.

  private void validateHAServerExceptions(Class<?> thriftInterface) {
    String className = thriftInterface.getClass().getName();
    Method[] methods = thriftInterface.getClass().getMethods();
    for (Method m : methods) {
      Class<?>[] exceptionClasses = m.getExceptionTypes();
      if (exceptionClasses.length == 0) {
        throw new IllegalStateException("Method " + m.getName() + " on " + className + ""
            + " does not declare ThriftNotActiveServiceException to be thrown");
      }
      boolean found = false;
      for (Class<?> ec : exceptionClasses) {
        if (ThriftNotActiveServiceException.class.getName().equals(ec.getClass().getName())) {
          found = true;
          break;
        }
      }
      if (!found) {
        throw new IllegalStateException("Method " + m.getName() + " on " + className + ""
            + " does not declare ThriftNotActiveServiceException to be thrown");        
      }
    }
  }

[keith-turner]

Re using reflection to look for absence of the exception, thrift one way methods can not throw an exception. Not sure if we can detect if a method is one way. If we could that would be nice in the HAService code, it could just ignore oneway thrift calls when not the primary instead of throwing an exception.

[dlmarion]

It looks like the information that's needed is in the Processor class, specifically a subclass that has the same name as the method, then you call isOneWay on it.

For example, for the ManagerClientService.Iface.waitForFlush method, there is a ManagerClientService.Processor.waitForFlush class, and its isOneWay method returns false.

[keith-turner]

@dlmarion in 8be2acf validated exceptions and handled one way methods.

[dlmarion]

I think you need to return on this line, or it will fall through and invoke the method.

[dlmarion]

I think you need to return on this line, or it will fall through and invoke the method.

[keith-turner]

Good catch, that would have spammed the logs w/ errors. Fixed in d5670e6

[dlmarion]

We probably want to log the class name for debugging purposes to find out why it doesn't have the declared method.