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

Send save actions in chunks to server #2947

Merged
merged 9 commits into from
Jul 30, 2018

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Jul 23, 2018

URL of deployed dev instance (used for testing):

Steps to test:

  • test saving-related actions (skeleton vs volume)
  • provoke large update actions (e.g., upload an NML with 100 000 tracings)
  • when saving, the save button should show a progress indicator

Issues:


Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Works beautifully :) LGTM!

Just out of curiosity: Saving after uploading a 200,000 node nml takes 1m7s for me on the deployed branch. Are your numbers comparable. I wonder if we can speed this up in a future PR :)

return (
<ButtonComponent
key="save-button"
type="primary"
onClick={this.props.onClick}
icon={this.getSaveButtonIcon()}
>
Save
{this.showProgress() ? (
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but would shouldShowProgress be a better name here as the function returns a boolean and doesn't execute an action.


const trees = generateDummyTrees(1000, 1);
Store.dispatch(addTreesAndGroupsAction(createTreeMapFromTreeArray(trees), []));
// t.true(true);
Copy link
Member

Choose a reason for hiding this comment

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

Leftover comment

t.is(state.save.queue[0].actions.length, maximumActionCountPerBatch);
});

test.serial("Save actions should be chunked after compacting (2/3)", t => {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be (3/3)?

});

test.serial("Save actions should be chunked after compacting (2/3)", t => {
const nodeCount = 20000;
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I was wondering what that parameter was, before :D

@@ -29,29 +37,55 @@ class SaveButton extends React.PureComponent<Props, State> {

savedPollingInterval: number = 0;
_forceUpdate = () => {
Copy link
Member

Choose a reason for hiding this comment

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

To get rid of the hourglass icon in the save button that is briefly displayed after saving, although the state was just successfully saved, we could call _forceUpdate after isBusy changed or was set to false (check for that in componentDidUpdate or so). Fine for me anyways, just wanted to mention it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! However, I'd like to postpone this until we get rid of the entire forceUpdate stuff (hopefully soon). It's quite tedious and maybe the flickering hourglass will serve as a reminder for us to improve this :)

@philippotto philippotto merged commit 28e85f5 into clean-up-merge-modal Jul 30, 2018
@philippotto philippotto deleted the chunk-save-actions branch August 13, 2018 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants