xds: migrate xDS resolver to use XdsClient APIs for watching individual LDS/RDS resources

[voidzcy]

No description provided.

[dapengzhang0]

I don't think this flag is necessary. It's immediately true once instantiated.

[voidzcy]

Ah, this was for the same reason as the rdsWatcher != this check for RDS watcher. When there is multi-threading involved, it is possible that a callback was scheduled (to be executed later) while this resolver is being shut down. Then when the callback is invoked, it should be ignored.

Without this flag, it would do the similar check resolveState != this.

[dapengzhang0]

I would recommend add the checks when you are really implementing the race handling, and review them together. This is really based on assumptions of future implementation which I cannot see the whole picture.

[voidzcy]

Okay, eliminated flags used for racing cases for now.

[dapengzhang0]

I believe start() is only called right after instantiation and won't be called twice.

[voidzcy]

The problem for not using start()/stop() is that the LdsResourceWatcher implementation class will not be self-contained. It would be something like

class XdsNameResolver {
  private LdsResourceWatcher ldsWatcher;

  @Override
  public void start(Listener2 listener) {
    ...
    ldsWatcher = new LdsResourceWatcherImpl();
    xdsClient.watchLdsResource(authority, ldsWatcher);
  }


  @Override
  public void shutdown() {
    ...
    if (xdsClient != null) {
      if (ldsWatcher != null) {
        xdsClient.cancelLdsResourceWatch(authority, ldsWatcher);
      }
      ...
    }
    ...
  }
}

I prefer the existing approach as it encapsulate things better. What do you think?

[dapengzhang0]

I was not saying not to use start()/stop(). It was about checking the started flag.

[dapengzhang0]

Let's not do this now based on assumptions on future XdsClient implementation. It's not clear how race is handled by XdsClient yet.

[voidzcy]

Eliminated. Will reconsider in next steps.

[dapengzhang0]

Should they be cleaned up in a FILO order?

[voidzcy]

It shouldn't matter. But sure, swapped.

[dapengzhang0]

I wonder where the error-prone warning was coming from in the old code.

[voidzcy]

It's for modifying ConcurrentMap clusterRefs in the loop. Not sure how that was triggered. But now, seems it doesn't trigger.