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

binlog: support binlog.pump.config configurations for pump and drainer #693

Merged
merged 30 commits into from
Aug 14, 2019

Conversation

weekface
Copy link
Contributor

@weekface weekface commented Jul 25, 2019

What problem does this PR solve?

  • support binlog.pump.config and binlog.drainer.config configurations for pump and drainer
  • support pump and drainer configuration rollout

What is changed and how does it work?

Check List

Tests

  • E2E test
  • Stability test

Code changes

  • Has Helm charts change
  • Has Go code change
  • Has documents change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Does this PR introduce a user-facing change?:

support `binlog.pump.config` and `binlog.drainer.config` configurations for pump and drainer 

@weekface weekface added this to the v1.0.0 milestone Jul 25, 2019
@weekface weekface requested review from tennix, aylei, xiaojingchen, csuzhangxc and DanielZhangQD and removed request for csuzhangxc July 25, 2019 07:08
heartbeat-interval = 2
[storage]
sync-log = true
stop-write-at-available-space = "100 gib"
Copy link
Contributor

Choose a reason for hiding this comment

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

# stop write when disk available space less then the configured size
# 42 MB -> 42000000, 42 mib -> 44040192
# default: 10 gib
# stop-write-at-available-space = "10 gib"

I think it should be 1 Gigabytes or less

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is available space size, using default.

# drainer configurations (change to the tags of your drainer version),
# just follow the format in the file and configure in the 'config' section
# as below if you want to customize any configuration.
config: |
Copy link
Contributor

@aylei aylei Jul 25, 2019

Choose a reason for hiding this comment

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

This causes the old configuration key invalid, which is an in-compatible change

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should document a migration guide for RC and beta users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is an in-compatible change like: #638, i think we should do it before GA. @tennix

Copy link
Contributor

Choose a reason for hiding this comment

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

What about we keep these two ways, but make it clear in the document that we do not maintain the config for the original way and users should migrate to the new way if they want to configure any new parameters that are not exposed in values.yaml.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we decided to make this change incompatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think? @tennix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with @DanielZhangQD we should not make this incompatible change.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep both for binlog configurations to keep compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments addressed.

@aylei
Copy link
Contributor

aylei commented Jul 25, 2019

I think rollout the binlog configuration is not a small change, can we put this to 1.1?

@weekface weekface modified the milestones: v1.0.0, v1.1 Jul 25, 2019
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@@ -26,6 +26,7 @@ done

/drainer \
-L={{ .Values.binlog.drainer.logLevel | default "info" }} \
-pd-urls={{ template "cluster.name" . }}-pd:2379 \
Copy link
Contributor

Choose a reason for hiding this comment

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

add http:// scheme

@@ -1,5 +1,6 @@
set -euo pipefail
/pump \
-pd-urls={{ template "cluster.name" . }}-pd:2379 \
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

2 similar comments
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

1 similar comment
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-in-kind

1 similar comment
@weekface
Copy link
Contributor Author

/run-e2e-in-kind

@xiaojingchen
Copy link
Contributor

/run-e2e-in-kind

@weekface weekface changed the title change tidb-binlog configration [DNM] change tidb-binlog configration Aug 13, 2019
@weekface
Copy link
Contributor Author

weekface commented Aug 13, 2019

@aylei @DanielZhangQD @tennix @xiaojingchen

This PR changed to support both configurations. PTAL again.

@weekface
Copy link
Contributor Author

/run-e2e-in-kind

@weekface weekface changed the title [DNM] change tidb-binlog configration binlog: support binlog.pump.config and binlog.drainer.config configurations for pump and drainer Aug 13, 2019
@weekface weekface changed the title binlog: support binlog.pump.config and binlog.drainer.config configurations for pump and drainer binlog: support binlog.pump/drainer.config configurations for pump and drainer Aug 13, 2019
@weekface weekface changed the title binlog: support binlog.pump/drainer.config configurations for pump and drainer binlog: support binlog.pump.config configurations for pump and drainer Aug 13, 2019
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

charts/tidb-cluster/values.yaml Outdated Show resolved Hide resolved
Co-Authored-By: Tennix <tennix@users.noreply.github.com>
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@weekface
Copy link
Contributor Author

/run-e2e-in-kind

@weekface weekface requested a review from aylei August 13, 2019 09:24
Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

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

LGTM

@weekface weekface merged commit a320e52 into pingcap:master Aug 14, 2019
@weekface weekface deleted the binlog-config branch August 14, 2019 02:19
@sre-bot
Copy link
Contributor

sre-bot commented Aug 14, 2019

cherry pick to release-1.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Aug 14, 2019

cherry pick to release-1.0 failed

weekface added a commit to weekface/tidb-operator that referenced this pull request Aug 14, 2019
weekface added a commit that referenced this pull request Aug 14, 2019
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.

7 participants