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

refactor(main): write the main control flow in an async function #129

Merged
merged 4 commits into from
Apr 17, 2018

Conversation

Haroenv
Copy link
Collaborator

@Haroenv Haroenv commented Feb 13, 2018

Not necessary, but found this easier to read

depends on #128

@Haroenv Haroenv requested a review from vvo February 13, 2018 11:39
@vvo
Copy link
Contributor

vvo commented Feb 13, 2018

conflict now

@Haroenv
Copy link
Collaborator Author

Haroenv commented Feb 13, 2018

conflict fixed.

Copy link
Contributor

@vvo vvo left a comment

Choose a reason for hiding this comment

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

Now that we have good settings handling, could you update the code to do a setSettings when bootstrap data is moved to production?

Settings must be set:

  • beginning of the script (like you did), usecase: we update settings on a running instance
  • end of bootstrap phase (after moveToProduction), usecase: handle copyIndex settings erase

No more needed to copy settings from production to bootstrap index now that all settings are in the code.

WDYT?

Also we need to check the memory consumption of the current code before deploying this fix. I have no idea of memory implication of doing this await code versus chained promises (in one case you have a big scope, in another you have one scope per task)

src/index.js Outdated
.catch(error);
async function main() {
await setSettings(mainIndex);
await setSettings(bootstrapIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why setSettings on the bootstrap index?

@Haroenv
Copy link
Collaborator Author

Haroenv commented Feb 13, 2018

Makes sense! I'll change it to what you suggested

@Haroenv
Copy link
Collaborator Author

Haroenv commented Feb 13, 2018

I set the settings on the bootstrap index as well so that if you use the search in the bootstrap phase, it will be set up correctly, and as a real follower (for quick QA)

Copy link
Collaborator Author

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

Can't request changes, but this needs to be rewritten for #179

@vvo
Copy link
Contributor

vvo commented Apr 13, 2018

Unsure about what's needed here?

@Haroenv
Copy link
Collaborator Author

Haroenv commented Apr 13, 2018

I never investigated what the memory impact is (see your comment about scope). Probably still gonna quickly refactor to be the new logic and merge next week

@Haroenv Haroenv merged commit 7c30925 into master Apr 17, 2018
@Haroenv Haroenv deleted the refactor/main-function branch April 17, 2018 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants