Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CCR] Edit follower index #28600

Closed

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jan 11, 2019

This PR adds the possibility to edit a follower index. It adds an action in the context menu of the table list.

For now, ES does not provide an API to read the advanced settings of a follower index, but they are working on one and we'll be able to populate the fields with the values.

INFO The base branch will be changed to feature/ccr once #28267 is merged.


screen shot 2019-01-14 at 15 09 54

screen shot 2019-01-14 at 15 10 06

screen shot 2019-01-14 at 15 13 14

@sebelga sebelga added Feature:CCR and Remote Clusters Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Jan 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sebelga sebelga force-pushed the feature/ccr_edit_follower_index branch from b37a51e to 5a0c7c0 Compare January 12, 2019 15:21
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sebelga sebelga force-pushed the feature/ccr_edit_follower_index branch from 5a0c7c0 to 7c7eb75 Compare January 14, 2019 06:12
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sebelga sebelga changed the title [CCR] WIP Edit follower index [CCR] Edit follower index Jan 14, 2019
@sebelga sebelga changed the base branch from feature/ccr to feature/ccr_seb-temp-1 January 14, 2019 14:29
@sebelga sebelga changed the base branch from feature/ccr_seb-temp-1 to feature/ccr January 14, 2019 14:33
@sebelga sebelga changed the base branch from feature/ccr to feature/ccr_seb-temp-1 January 14, 2019 14:34
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, and everything seems to work as expected!

I had some suggestions for ways I think we can make the code clearer, but nothing critical.

In terms of the UI, I think the "Not found" state could be improved:

image

Instead of using a callout, could we use the same pattern as Remote Clusters? I think the call out means that something went wrong, but in this case there wasn't a mistake or problem, we're just telling the user that the thing they expected to be there, wasn't.

image

Instead of using the primary button, could we use EuiButtonEmpty with an left-facing arrow icon, and align it to the left, and change the copy to indicate that we're going back in the nav hierarchy? "View" sounds like we're going to jump somewhere else, possibly to another app.

image

<EuiButtonEmpty
  {...routing.getRouterLinkProps('/follower_indices')}
  iconType="arrowLeft"
  flush="left"
>
  <FormattedMessage
    id="xpack.crossClusterReplication.followerIndexEditForm.viewFollowerIndicesButtonLabel"
    defaultMessage="Back to follower indices"
  />
</EuiButtonEmpty>

disabled: !isConnected,
'data-test-subj': `option-${name}`
}));
const remoteClustersOptions = this.props.remoteClusters

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this style was placed back after merging

componentDidUpdate(prevProps, prevState) {
const { followerIndex, getFollowerIndex } = this.props;
if (!followerIndex && prevState.lastFollowerIndexId !== this.state.lastFollowerIndexId) {
// Fetch the auto-follow pattern on the server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: auto-follow pattern -> follower index


componentDidUpdate(prevProps, prevState) {
const { followerIndex, getFollowerIndex } = this.props;
if (!followerIndex && prevState.lastFollowerIndexId !== this.state.lastFollowerIndexId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this code guards against reloading an index that's already been loaded, but when will this scenario occur? If we call getFollowerIndex(this.state.lastFollowerIndexId) within componentDidMount will that remove the need for this check and for state.lastFollowerId?

}

saveFollowerIndex = (name, followerIndex) => {
this.inflightPayload = { name, followerIndex };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inflightPayload seems like a misleading name, since the payload isn't in flight yet. How about changing this to stagedFollowerIndex, saveableFollowerIndex, or followerIndexToSave?

const { followerIndexId, intl } = this.props;
const title = intl.formatMessage({
id: 'xpack.crossClusterReplication.followerIndexEditForm.confirmModal.title',
defaultMessage: 'Update follower index \'{id}\'',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be phrased as a question, per the EUI docs.

<EuiIcon type="alert" color="danger" />{' '}
<FormattedMessage
id="xpack.crossClusterReplication.followerIndexEditForm.confirmModal.description"
defaultMessage="To update the follower index, it will first be paused and then resumed."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to give the user information, but maybe we need to provide more so that the user feels in control of this situation.

Is there the potential for a problem, e.g. the resume request could fail, leaving the follower index in a paused state? In that case we could say something like, "To update the follower index, it will first be paused and then resumed. If the update fails, you may need to manually resume the follower index."

@@ -84,8 +84,10 @@ class Routing {
return encodeURI(`#${BASE_PATH}/auto_follow_patterns${section}/${encodeURIComponent(name)}`);
};

getFollowerIndexPath = (name, section = '/edit') => {
return encodeURI(`#${BASE_PATH}/follower_indices${section}/${encodeURIComponent(name)}`);
getFollowerIndexPath = (name, section = '/edit', withBase = true) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about rewriting this to be goToEditFollowerIndexPath to make it a bit more convenient?

goToEditFollowerIndex = (name) => {
  const uri = encodeURI(`#${BASE_PATH}/follower_indices/edit/${encodeURIComponent(name)}`);
  this.navigate(uri);
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just returning a string is more flexible for using the uri in other contexts, like href

handler: async () => (
await getFollowerIndexRequest(id)
)
});

export const saveFollowerIndex = (name, followerIndex) => (
export const saveFollowerIndex = (name, followerIndex, isUpdating) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how we're unifying the behavior of the create and update actions -- that's really nice for maintainability! I think there's a way we can simplify both the interface and implementation.

In the past I've found that conditional flags like isUpdating tend to bloat the function signature, make the call sites more difficult to follow, and also add complexity to the implementation in the form of with parallel branches split across multiple conditions.

I think we can simplify the code and make it clearer by exporting separate functions for create and update, and extracting the common logic into an internal function. Does this seem like an improvement to you?

const saveFollowerIndex = (name, followerIndex, handler, successMessage) => (
  sendApiRequest({
    label: t.FOLLOWER_INDEX_CREATE,
    status: API_STATUS.SAVING,
    scope: `${scope}-save`,
    handler,
    onSuccess() {
      toastNotifications.addSuccess(successMessage);
      routing.navigate(`/follower_indices`, undefined, {
        pattern: encodeURIComponent(name),
      });
    },
  })
);

export const createFollowerIndex = (name, followerIndex) => (
  const handler = async () => {
    return await createFollowerIndexRequest({ name, ...followerIndex });
  };

  const successMessage = i18n.translate('xpack.crossClusterReplication.followerIndex.addAction.successNotificationTitle', {
    defaultMessage: `Added follower index '{name}'`,
    values: { name },
  });

  saveFollowerIndex(name, followerIndex, handler, successMessage);
);

export const updateFollowerIndex = (name, followerIndex) => (
  const handler = async () => {
    return await updateFollowerIndexRequest(name, followerIndex);
  };

  const successMessage = i18n.translate('xpack.crossClusterReplication.followerIndex.updateAction.successNotificationTitle', {
    defaultMessage: `Follower index '{name}' updated successfully`,
    values: { name },
  });

  saveFollowerIndex(name, followerIndex, handler, successMessage);
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am deferring this for now since both follower index and auto follow pattern actions perform create and update with one save method, and I don't want to introduce any regressions this late 😄

try {
/**
* We need to first pause the follower and then resume it passing the advanced settings
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really minor nit, but my OCD kicks in when we mix our comment styles. 😅 Could we stick with // for inline comments?

* We need to first pause the follower and then resume it passing the advanced settings
*/
// Pause follower
await callWithRequest('ccr.pauseFollowerIndex', { id: _id });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to wrap the pause and resume requests in individual try/catches, and then respond with errors that provides more insight into what happened? For example, if the pause fails, we could add a special code that will allow the UI to explain "Your follower index could not be paused, so it wasn't updated" and then provide the underlying ES error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a nice enhancement, but deferring since it's an edge case

title={(
<FormattedMessage
id="xpack.crossClusterReplication.followerIndex.editTitle"
defaultMessage="Edit follower index"

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remote clusters has been updated to just say Edit remote cluster 😄

@jen-huang jen-huang changed the base branch from feature/ccr_seb-temp-1 to feature/ccr January 18, 2019 19:34
@jen-huang
Copy link
Contributor

Closing this PR in favor of #28778, all PR feedback will be addressed there instead.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:CCR and Remote Clusters Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants