Do not perform unnecessary round-trip to ES. #7960

thomasneirynck · 2016-08-09T03:14:15Z

In place fix for #7945.

Skips ES fetch when there are no changes in the request-parameters after pressing the "play" button in the side-bar.

thomasneirynck · 2016-08-09T19:22:43Z

jenkins, test this

thomasneirynck · 2016-08-09T19:43:54Z

src/core_plugins/kibana/public/visualize/editor/editor.js

@@ -293,14 +294,21 @@ uiModules

  function transferVisState(fromVis, toVis, stage) {
    return function () {
+


this function is invoked with the angular-binding in sidebar.html (cf. stageEditableVis)

cjcenizal · 2016-08-09T20:58:00Z

jenkins test this

cjcenizal · 2016-08-09T21:10:21Z

src/core_plugins/kibana/public/visualize/editor/editor.js

+        $scope.fetch();
+      } else {
+        $state.save();
+      }


Small suggestion. I think we make this a little more readable by nesting conditions, exiting early, computing values near where they're needed, and putting comments on their own line:

if (stage) { // Only requery ES when the query has changed. vislib params are irrelevant. const fromVisLib = getVisLibOptions(fromVis); const toVisLib = getVisLibOptions(toVis); if (_.isEqual(toVisLib, fromVisLib) { return $scope.fetch(); } } $state.save();

I'm also not sure what that comment means in the context of that condition... can you explain some more?

epixa · 2016-08-09T21:56:42Z

I'm not totally sure, but we had an issue with master earlier, so this build may pass if you rebase.

If the visualization parameters change, but the query stays intact, we should not fetch a new query.

thomasneirynck · 2016-08-10T13:58:00Z

jenkins, test this

thomasneirynck · 2016-08-10T18:25:24Z

failing screenshot test after rebasing. Need to investigate ...

The below is a more clear way to express what needs to happen. This also avoids a bug when there is no re-fetch when there are both changes in the Options and Data tab.

thomasneirynck · 2016-08-10T22:59:19Z

@cjcenizal can you check once more? I made a change to the original you reviewed. The play-button triggers a visualization when there is a change in the "Data"-tab. (Instead of 'no changes in the "Options" tab').

@jbudz @epixa Can one of you review as well?

jbudz · 2016-08-11T14:32:53Z

src/ui/public/vis/agg_configs.js

+   * Ignores the non-array indexes
+   * @param aggConfigs an AggConfigs instance
+   */
+  AggConfigs.prototype.jsonDataEquals = function (aggConfigs) {


can we add a test for this?

jbudz · 2016-08-11T14:50:35Z

Functionally this LGTM, some comments above about testing.

cjcenizal · 2016-08-11T17:13:32Z

src/core_plugins/kibana/public/visualize/editor/editor.js

@@ -293,14 +293,25 @@ uiModules

  function transferVisState(fromVis, toVis, stage) {
    return function () {
+
+      //verify this before we copy the "new" state
+      const aggregationChanges = !fromVis.aggs.jsonDataEquals(toVis.aggs);


This might be clearer if we prefix with is, has, or should to indicate the boolean nature of the var. How about isAggregationsChanged?

cjcenizal · 2016-08-11T17:17:11Z

Nice work! Just had a couple readability-related suggestions.

Also test .toJSON orders properties consistently.

cjcenizal · 2016-08-11T17:29:15Z

Ahhh, great names! So much more context! Thank you. LGTM!

thomasneirynck · 2016-08-11T18:11:36Z

jenkins, test this

jbudz · 2016-08-11T18:39:11Z

LGTM

Do not perform unnecessary round-trip to ES. Former-commit-id: 0c9e2af

`v95.7.0` ⏩ `v95.9.0` > [!CAUTION] > This PR contains the final set of Emotion conversions for all EuiForm components. > If your plugin contains any custom CSS/styling to **EuiFormRow, EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**, ⚠️ make sure to QA your UI to ensure no visual regressions have occurred! ⚠️ --- ## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0) - Updated `EuiSearchBar`'s optional `box.schema` prop with a new `recognizedFields` configuration. This allows specifying the phrases that will be parsed as field clauses ([#7960](elastic/eui#7960)) - Updated `EuiIcon` with a new `tokenSemanticText` glyph ([#7971](elastic/eui#7971)) - Added support for TypeScript 5 ([#7980](elastic/eui#7980)) **Bug fixes** - Fixed `EuiSelectableTemplateSitewide` styles when used within a dark-themed `EuiHeader` ([#7977](elastic/eui#7977)) ## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0) - Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding by default ([#7961](elastic/eui#7961)) - This can be overridden via `popoverProps.panelPaddingSize` if needed. - Updated `EuiHeaderLink` to default to a size of `s` (down from `m`) ([#7961](elastic/eui#7961)) **Accessibility** - Updated the `aria-label` attribute for the `EuiFieldSearch` clear button ([#7970](elastic/eui#7970)) **Bug fixes** - Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover" />` form controls ([#7957](elastic/eui#7957)) **Deprecations** - Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use `columnCompressed` instead, which will automatically account for child `EuiSwitch`es ([#7968](elastic/eui#7968)) - Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row` instead for vertical forms, or `centerCompressed` for inline forms ([#7968](elastic/eui#7968)) - (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no longer attempt to automatically align its content to a vertical center. Use the `display="center"` prop for that instead ([#7968](elastic/eui#7968)) **CSS-in-JS conversions** - Converted `EuiFormControlLayout` to Emotion ([#7954](elastic/eui#7954)) - Removed `.euiFormControlLayout--*icons` classNames and `--eui-form-control-layout-icons-padding` CSS var. Use `--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount` instead - Converted `EuiFormLayoutDelimited` to Emotion ([#7957](elastic/eui#7957)) - Fixed `cloneElementWithCss` throwing an error when used multiple times without a `key` prop ([#7957](elastic/eui#7957)) - Updated `cloneElementWithCss` utility to support a third argument that allows prepending vs. appending the cloned Emotion css className ([#7957](elastic/eui#7957)) - Removed `@euiFormControlLayoutClearIcon` Sass mixin ([#7959](elastic/eui#7959)) - Converted `EuiDescribedFormGroup` to Emotion ([#7964](elastic/eui#7964)) - Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to Emotion ([#7966](elastic/eui#7966)) - Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed `@euiFormLabel` mixin ([#7967](elastic/eui#7967)) - Converted `EuiFormRow` to Emotion ([#7968](elastic/eui#7968)) - Converted `EuiCheckbox` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiRadio` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiSwitch` to Emotion ([#7969](elastic/eui#7969)) - Removed the following Sass variables: ([#7969](elastic/eui#7969)) - `$euiFormCustomControlDisabledIconColor` - `$euiFormCustomControlBorderColor` - `$euiRadioSize` - `$euiCheckBoxSize` - `$euiCheckboxBorderRadius` - `$euiSwitchHeight` (and compressed/mini variants) - `$euiSwitchWidth` (and compressed/mini variants) - `$euiSwitchThumbSize` (and compressed/mini variants) - `$euiSwitchIconHeight` - `$euiSwitchOffColor` - Removed the following Sass mixins: ([#7969](elastic/eui#7969)) - `euiIconBackground` - `euiCustomControl` - `euiCustomControlSelected` - `euiCustomControlDisabled` - `euiCustomControlFocused` --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>

thomasneirynck reviewed Aug 9, 2016
View reviewed changes

thomasneirynck added review v5.0.0-beta1 labels Aug 9, 2016

thomasneirynck assigned epixa and cjcenizal Aug 9, 2016

cjcenizal reviewed Aug 9, 2016
View reviewed changes

thomasneirynck removed the review label Aug 9, 2016

Do not perform unnecessary round-trip to ES.

3f59e38

If the visualization parameters change, but the query stays intact, we should not fetch a new query.

thomasneirynck force-pushed the fix/7945 branch 2 times, most recently from 64dfe4b to 0b13617 Compare August 10, 2016 13:33

thomasneirynck force-pushed the fix/7945 branch from 0b13617 to 8e8721c Compare August 10, 2016 22:02

Only refresh when Data-tab has changed

433c842

The below is a more clear way to express what needs to happen. This also avoids a bug when there is no re-fetch when there are both changes in the Options and Data tab.

thomasneirynck force-pushed the fix/7945 branch from 8e8721c to 433c842 Compare August 10, 2016 22:25

thomasneirynck assigned jbudz Aug 10, 2016

jbudz reviewed Aug 11, 2016
View reviewed changes

cjcenizal reviewed Aug 11, 2016
View reviewed changes

Add tests for jsonDataEquals

ca6ce25

Also test .toJSON orders properties consistently.

thomasneirynck force-pushed the fix/7945 branch from 4fb8601 to ca6ce25 Compare August 11, 2016 17:26

thomasneirynck merged commit 0c9e2af into elastic:master Aug 11, 2016

thomasneirynck mentioned this pull request Aug 11, 2016

Visualizations are instantiated twice when changing options #7945

Closed

epixa mentioned this pull request Aug 23, 2016

Show warning notification when sorting in asc order by count in term agg #8050

Merged

thomasneirynck mentioned this pull request Sep 16, 2016

changing per page setting does not rearrange rows in datatable #8322

Closed

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#7960 from thomasneirynck/fix/7945

cee71c1

Do not perform unnecessary round-trip to ES. Former-commit-id: 0c9e2af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not perform unnecessary round-trip to ES. #7960

Do not perform unnecessary round-trip to ES. #7960

thomasneirynck commented Aug 9, 2016 •

edited

Loading

thomasneirynck commented Aug 9, 2016

thomasneirynck Aug 9, 2016

cjcenizal commented Aug 9, 2016

cjcenizal Aug 9, 2016 •

edited

Loading

epixa commented Aug 9, 2016

thomasneirynck commented Aug 10, 2016

thomasneirynck commented Aug 10, 2016

thomasneirynck commented Aug 10, 2016 •

edited

Loading

jbudz Aug 11, 2016

jbudz commented Aug 11, 2016

cjcenizal Aug 11, 2016

cjcenizal commented Aug 11, 2016

cjcenizal commented Aug 11, 2016

thomasneirynck commented Aug 11, 2016

jbudz commented Aug 11, 2016

		@@ -293,14 +294,21 @@ uiModules

		function transferVisState(fromVis, toVis, stage) {
		return function () {

Do not perform unnecessary round-trip to ES. #7960

Do not perform unnecessary round-trip to ES. #7960

Conversation

thomasneirynck commented Aug 9, 2016 • edited Loading

thomasneirynck commented Aug 9, 2016

thomasneirynck Aug 9, 2016

Choose a reason for hiding this comment

cjcenizal commented Aug 9, 2016

cjcenizal Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

epixa commented Aug 9, 2016

thomasneirynck commented Aug 10, 2016

thomasneirynck commented Aug 10, 2016

thomasneirynck commented Aug 10, 2016 • edited Loading

jbudz Aug 11, 2016

Choose a reason for hiding this comment

jbudz commented Aug 11, 2016

cjcenizal Aug 11, 2016

Choose a reason for hiding this comment

cjcenizal commented Aug 11, 2016

cjcenizal commented Aug 11, 2016

thomasneirynck commented Aug 11, 2016

jbudz commented Aug 11, 2016

thomasneirynck commented Aug 9, 2016 •

edited

Loading

cjcenizal Aug 9, 2016 •

edited

Loading

thomasneirynck commented Aug 10, 2016 •

edited

Loading