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

♻️ Fix compressor readiness shutdown_duration / Fix cassandra … #376

Conversation

rinx
Copy link
Contributor

@rinx rinx commented May 14, 2020

…serial_consistency value

Signed-off-by: Rintaro Okamura rintaro.okamura@gmail.com

Description:

I fixed several fields in values.yaml.

Related Issue:

nothing

How Has This Been Tested?:

nothing

Environment:

  • Golang Version: 1.14
  • Docker Version: 19.03.5
  • Kubernetes Version: 1.17.3
  • NGT Version: 1.9.1

Types of changes:

  • Bug fix [type/bug]
  • New feature [type/feature]
  • Add tests [type/test]
  • Security related changes [type/security]
  • Add documents [type/documentation]
  • Refactoring [type/refactoring]
  • Update dependencies [type/dependency]
  • Update benchmarks and performances [type/bench]
  • Update CI [type/ci]

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked open Pull Requests for the similar feature or fixes?
  • I have added tests and benchmarks to cover my changes.
  • I have ensured all new and existing tests passed.
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly.

…consistency value

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>
@pull-assistant
Copy link

pull-assistant bot commented May 14, 2020

Score: 0.67

Best reviewed: commit by commit


Optimal code review plan

     ♻️ Fix compressor readiness shutdown_duration / Fix cassandra s...

     🔧 enable compressor's liveness and set maxUnavailable to 1

Powered by Pull Assistant. Last update aaabf2b ... 15787e9. Read the comment docs.

readiness:
server:
http:
shutdown_duration: 1m
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think readiness should shutdown immediately. this setting increase duration until disconnect from kubernetes service dns.

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you please describe more about why we need 1minutes duration for shutting down the readiness server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm. which field affects to make compressor pods alive longer without increasing duration until disconnect?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uh, I see. readiness should shutdown immediately so it should be 0s and for liveness, it affects pod to keep them alive.
but I know, we don't use liveness probe for agent and compressor, hmm.... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ha, okay understood.
we have now a backup strategy in compressor post-stop, it may be okay to be terminated suddenly caused by unexpected fails of liveness servers. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so, maybe we need another phase for shutting down the process in internal server

Copy link
Collaborator

Choose a reason for hiding this comment

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

if we can set post pre process for each server, It would be useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see. for now, i'm going to enable liveness and set readiness shutdown_duration to be zero. thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

sound good to me.
please set liveness shutdown duration over 1minutes.
and we need to think about pod disruption budget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised. please check it 😄

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>
Copy link
Collaborator

@kpango kpango left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango merged commit 4a8bccf into master May 14, 2020
@kpango kpango deleted the refactor/helm/fix-default-values-compressor-readiness-and-cassandra-serial-consistency branch May 14, 2020 02:15
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