Skip to content

Conversation

@feedmeapples
Copy link
Contributor

@feedmeapples feedmeapples commented Jul 27, 2020

What changed?
development-*.yaml docker configs now set archival feature to "disabled" by default.
Added development_archival.yaml that has archival "enabled"

Why?
File based implementation of archival feature may not be the best to be enabled by default as it can bloat up the storage.

How did you test it?

  • Built a new docker image
    docker build -t temporalio/server:test .
  • Span it up using docker-compose up
  • tctl namespace desribe
    image

Potential risks
No risks

@feedmeapples feedmeapples marked this pull request as draft July 27, 2020 15:30
@feedmeapples feedmeapples changed the title Disable Archival feture in development docker, add separate config fo… Disable Archival feature in development docker Jul 27, 2020
@feedmeapples feedmeapples marked this pull request as ready for review July 27, 2020 15:58
@feedmeapples feedmeapples marked this pull request as draft July 27, 2020 19:01
Copy link
Contributor

@samarabbas samarabbas left a comment

Choose a reason for hiding this comment

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

You want to also update config_template.yaml used by docker builds:
https://github.com/temporalio/temporal/blob/master/docker/config_template.yaml

@samarabbas samarabbas requested a review from markmark206 July 27, 2020 19:56
@feedmeapples feedmeapples marked this pull request as ready for review July 27, 2020 20:06
@feedmeapples feedmeapples requested a review from samarabbas July 27, 2020 21:58
policy: "noop"
toDC: ""

archival:
Copy link

@markmark206 markmark206 Jul 28, 2020

Choose a reason for hiding this comment

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

just curious – were you able to start temporal server using this config file? if not, i wonder if it is worth it to postpone adding development_archival.yaml until we release tested/supported archival feature and have a chance to really test this. (not a blocker for this merge, of course, just thinking out loud).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markmark206 this configuration is the same as the previous development.yaml which had archival enabled and that we've been running. Should we do a separate testing of it?

Choose a reason for hiding this comment

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

we talked about testing all config files at some point, but until then, just running ./temporal-server -e development_archival start would be enough. (since this is a copy paste of another file, it's likely it just works, and it's not breaking any workflows, so i think this is also fine as is.)

Copy link
Contributor Author

@feedmeapples feedmeapples Jul 29, 2020

Choose a reason for hiding this comment

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

tested and i can see the default archival state set to Enabled if started using development_archival config

Registering new namespace (tctl --namespace n1 namespace register) results in this state of archival:
image

archival:
history:
state: "enabled"
state: "disabled"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you file a separate issue to make this docker parameter which users can control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a ticket #623 Add control of archival feature initial state through docker-compose parameter.

Does this mean that the variable will override the archival state set in defaultNamespace of temporal config?

@feedmeapples feedmeapples linked an issue Jul 28, 2020 that may be closed by this pull request
@feedmeapples feedmeapples merged commit 4b01234 into temporalio:master Jul 29, 2020
@feedmeapples feedmeapples deleted the issue596 branch August 19, 2020 20:14
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.

Disable archival by default for temporal docker

3 participants