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

Make ESM Instance ID Configurable #61

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Make ESM Instance ID Configurable #61

merged 1 commit into from
Jun 23, 2020

Conversation

lornasong
Copy link
Member

@lornasong lornasong commented May 12, 2020

Feature request to allow practitioners to set instance ids in order for ESM
instances to terminate and reregister with Consul with the same id. Ungraceful
terminations currently take 30 minutes to be reaped. Without this feature, when
a new instance starts up, it has a new id and leads to multiple ESM instances
rather than 'replacing' the terminated one.

  • Add instance_id in configuration file
  • Replace existing id used for testing with instance_id
  • Add a check to ensure instance_id is unique since we are no longer generating
    a unique id on behalf of the practitioner
  • Move creating a UUID for id into DefaultConfig() to prevent race condition

Resolves #60

@lornasong lornasong requested a review from a team May 12, 2020 21:31
Copy link
Contributor

@findkim findkim left a comment

Choose a reason for hiding this comment

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

The changes look great, and allowing the ID to be configurable would be useful for continuity from a monitoring perspective.

Could you add the new config field to TestDecodeMergeConfig as a small sanity test? Going to go ahead and approve this 👍

@lornasong
Copy link
Member Author

@findkim - great point, I added the new config to the TestDecodeMergeConfig. Thanks for the review and all the helpful feedback!

@lornasong lornasong force-pushed the issue-60-config-id branch from 19d9e08 to a263589 Compare June 23, 2020 20:36
Feature request to allow practitioners to set instance ids in order for ESM
instances to terminate and reregister with Consul with the same id. Ungraceful
terminations currently take 30 minutes to be reaped. Without this feature, when
a new instance starts up, it has a new id and leads to multiple ESM instances
rather than 'replacing' the terminated one.

- Add instance_id in configuration file
- Replace existing `id` used for testing with instance_id
- Add a check to ensure instance_id is unique since we are no longer generating
a unique id on behalf of the practitioner
@lornasong lornasong force-pushed the issue-60-config-id branch from a25005a to 2e60fae Compare June 23, 2020 20:55
@lornasong lornasong merged commit 7face0b into master Jun 23, 2020
@lornasong lornasong deleted the issue-60-config-id branch June 23, 2020 21:00
@lornasong lornasong added this to the 0.4.0 milestone Jul 24, 2020
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.

Make ESM Instance ID Configurable
2 participants