Update Delete Remote Cluster API to support multiple comma-delimited clusters.

[cjcenizal]

This fixes a bug in how success and failure status is reported in the UI when you delete multiple remote clusters. Originally there was a race condition which prevented accurate reporting of this status.

[elasticmachine]

Pinging @elastic/es-ui

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[cjcenizal]

This is an error because no promise is returned, so removeClusterRequests didn't represent the actual requests being sent. This resulted in a race condition on line 35, because we only wait as long as the timing promise on line 38 takes to resolve instead of waiting for our API requests to resolve.

As a result, the toasts we display won't be accurate reflections of what was deleted if the requests take longer to resolve than 500ms. I never caught this bug because I never tested the UI with a high-latency connection (simulated or otherwise).

While I was fixing this, I also noticed how inefficient this implementation is because it sends out multiple requests. I decided to adopt the approach we've taken elsewhere by updating the API endpoint to support multiple remote clusters in a single DELETE request.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[sebelga]

Thanks for spotting this bug and fixing it @cjcenizal ! It works very well but it was a bit hard to read the server handler function. I made a suggestion to what I think makes it easier to follow.
I also think the client action could be simplified a bit by using the same variable names that the request response. Let me know what you think 😊

[sebelga]

Why not use the same variable names as the request response? ("itemsDeleted" and "errors").
Also, I removed the 500ms timeout and I can't not see the flicker in the comment.

I also wonder if we shouldn't have a catch in case the server throws an error...

dispatch({
  type: REMOVE_CLUSTERS_START,
});

const clustersToDelete = names.join(',');
const { itemsDeleted, errors } = await sendRemoveClusterRequest(clustersToDelete)
  .then(({ data }) => data)
  .catch(() => ({ errors: clustersToDelete, itemsDeleted: [] }));

if (errors.length > 0) {
   ...

[sebelga]

I think it would be clearer to extract the function and give it a name (like I did in rollup https://github.com/elastic/kibana/blob/master/x-pack/plugins/rollup/server/routes/api/jobs.js#L119).

I also had a hard time to read it, I would suggest to split it in parts. What do you think about this:

handler: async (request) => {
  const callWithRequest = callWithRequestFactory(server, request);
  const { name } = request.params;
  const names = name.split(',');

  const itemsDeleted = [];
  const errors = [];

  // Validator that returns an error if the remote cluster does not exist
  const validateClusterDoesExist = async (name) => {
    try {
      const existingCluster = await doesClusterExist(callWithRequest, name);
      return existingCluster
        ? null
        : wrapCustomError(new Error(`The cluster ${name} does not exist.`), 404);
    } catch (err) {
      return wrapCustomError(err, 400);
    }
  };

  // Send the request to delete the cluster and returns an error if it could not be deleted
  const sendRequestToDeleteCluster = async (name) => {
    const body = serializeCluster({ name });

    try {
      const response = await callWithRequest('cluster.putSettings', { body });
      const acknowledged = get(response, 'acknowledged');
      const cluster = get(response, `persistent.cluster.remote.${name}`);

      if (acknowledged && !cluster) {
        return null;
      }

      // If for some reason the ES response still returns the cluster information,
      // return an error. This shouldn't happen.
      return wrapCustomError(new Error('Unable to delete cluster, information still returned from ES.'), 400);
    } catch(err) {
      return isEsError(err)
        ? wrapEsError(err)
        : wrapUnknownError(err);
    }
  };

  const deleteCluster = async (clusterName) => {
    try {
      // Validate that the cluster exists
      let error = await validateClusterDoesExist(clusterName);

      if (!error) {
        // Delete the cluster
        error = await sendRequestToDeleteCluster(clusterName);
      }

      if (error) {
        throw error;
      }

      // If we are here, it means that everything went well...
      itemsDeleted.push(clusterName);
    } catch(err) {
      errors.push({ name: clusterName, error: err });
    }
  };

  // Delete all our cluster in parallel
  await Promise.all(names.map(deleteCluster));

  return {
    itemsDeleted,
    errors,
  };
}

[cjcenizal]

I like this much better! Thanks for the great suggestion.

[cjcenizal]

Thanks for the review @sebelga! I've implemented your suggestions. Can you please take another look?

Regarding the flicker, I'm able to reproduce it regularly. If the request resolves more quickly than 500ms then I see it.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[sebelga]

LGTM! Thanks for updating the handler. I'm not convinced by the new variable name but hey... it's just a variable name (one of the two hard things in computer science!) 😄

[sebelga]

It would be interesting to know why this app requires this hack and others don't. Let's leave it for now and you show it to me over zoom at some point. 😊

[cjcenizal]

Ideally we should do this anywhere we display a loading state and we care about a flicker disrupting the UX, though AFAIK it's inconsistently implemented. Here's how it looks without a minimum delay:

[sebelga]

I see... it's kind of the React scheduler that we will get. I might have solved it differently to avoid adding a hack on each http request.
Wrap the full-screen eui loader with an HOC that sets a timeout of 300ms, and only after that time it shows the component. If it's less than 300 ms, don't do anything.

This way, all of Kibana benefit from it and we don't have to add Promise.all.

[sebelga]

Nice improvement!

[sebelga]

This seems over necessary to me. If you prefer to call it names (plural) to make it clearer that it can accept more than 1 item, we could (although then we'd have a problem of naming with the local var).

This pattern is very common in Javascript to have to explicitly write it in the variable name

function something(id: string | string[]) {
  const ids = arrify(id);
   ...
}

[cjcenizal]

I like the pattern you're proposing, but in your example the input is an array or string that gets coerced to an array. In this case, the input is always a string, which we're splitting into an array. I'm not sure I see how the pattern can be applied here -- what am I missing?

[sebelga]

Sorry, I was missing the fact that indeed this is different from the example I was giving.
I guess that here, if the name of the param was names, and I read this code

const { names } = request.params;
const namesArray = names.split(',');

// At this point it is clear that we have an array of names (one or many is not important)
....

If this was a public API, the doc would clearly specify that the name can be a list of names separated by a comma. And here as a private API I am not sure that being that explicit brings a lot. But as I said in my first comment, I don't feel strongly about this 😊

[cjcenizal]

Thanks for the review @sebelga! I'm going to merge this for the sake of progress but feel free to address #34595 (comment) and I can incorporate your feedback in another PR.