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.

Sign up for GitHub

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

Jump to bottom

etcdserver: add --max-txn-ops flag #7976

Merged
merged 2 commits into from
May 25, 2017
Merged

etcdserver: add --max-txn-ops flag #7976

merged 2 commits into from
May 25, 2017

Conversation

fanminshi
Copy link
Member

Fixes #7826

heyitsanthony
heyitsanthony reviewed May 24, 2017
View reviewed changes
etcdmain/config.go Outdated
@@ -138,6 +138,7 @@ func newConfig() *config {
fs.UintVar(&cfg.TickMs, "heartbeat-interval", cfg.TickMs, "Time (in milliseconds) of a heartbeat interval.")
fs.UintVar(&cfg.ElectionMs, "election-timeout", cfg.ElectionMs, "Time (in milliseconds) for an election to timeout.")
fs.Int64Var(&cfg.QuotaBackendBytes, "quota-backend-bytes", cfg.QuotaBackendBytes, "Raise alarms when backend size exceeds the given quota. 0 means use the default quota.")
fs.UintVar(&cfg.MaxOpsPerTxn, "max-ops-per-txn", cfg.MaxOpsPerTxn, "maximum operations per txn that etcd server allows (0 is unlimited); defaults to unlimited.")
Copy link
Contributor

Choose a reason for hiding this comment

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

max-txn-ops; the per is redundant. Should also probably default to 128 so it's harder to take down etcd with the default settings.

heyitsanthony
heyitsanthony reviewed May 24, 2017
View reviewed changes
etcdserver/api/v3rpc/key.go Outdated
@@ -39,6 +41,7 @@ type kvServer struct {
}

func NewKVServer(s *etcdserver.EtcdServer) pb.KVServer {
MaxOpsPerTxn = int(s.Cfg.MaxOpsPerTxn)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will break embedding when using multiple etcd servers with different max txn op configurations

Copy link
Member Author

@fanminshi fanminshi May 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@heyitsanthony I am thinking about each KvServer should hold its own MaxOpsPerTxn stored in

type kvServer struct {
	hdr header
	kv  etcdserver.RaftKV
        maxTxnOps uint
}

and use kvServer.maxTxnOps for transaction ops check?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@gyuho gyuho added this to the v3.3.0 milestone May 24, 2017
@fanminshi fanminshi force-pushed the make_maxOpsPerTxn_configurable branch from 76a3659 to 66b29a4 Compare May 24, 2017 17:31
@fanminshi
etcdserver: add --max-txn-ops flag
ae7ddfb 
--max-txn-ops allows users to define the maximum transaction operations
for each txn request. it defaults at 128.

Fixes etcd-io#7826
@fanminshi fanminshi force-pushed the make_maxOpsPerTxn_configurable branch from 66b29a4 to ae7ddfb Compare May 24, 2017 17:32
@fanminshi
Copy link
Member Author

All fixed. PTAL

@fanminshi fanminshi changed the title etcdserver: add --max-ops-per-txn flag etcdserver: add --max-txn-ops flag May 24, 2017
@fanminshi fanminshi force-pushed the make_maxOpsPerTxn_configurable branch from 2479cb9 to 34b8bad Compare May 24, 2017 18:47
@fanminshi
Copy link
Member Author

fixed CI breakage.

heyitsanthony
heyitsanthony suggested changes May 24, 2017
View reviewed changes
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

lgtm after fixing last few nits

etcdserver/api/v3rpc/key.go Outdated
func checkTxnRequest(r *pb.TxnRequest) error {
if len(r.Compare) > MaxOpsPerTxn || len(r.Success) > MaxOpsPerTxn || len(r.Failure) > MaxOpsPerTxn {
func checkTxnRequest(r *pb.TxnRequest, maxTxnOps int) error {
plog.Infof("maxTxnOps %v", maxTxnOps)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

etcdmain/help.go Outdated
@@ -66,6 +66,8 @@ member flags:
comma-separated whitelist of origins for CORS (cross-origin resource sharing).
--quota-backend-bytes '0'
raise alarms when backend size exceeds the given quota (0 defaults to low space quota).
--max-txn-ops '128'
maximum operations per txn that etcd server allows; defaults to 128.
Copy link
Contributor

Choose a reason for hiding this comment

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

the default is already given in the line above, don't restate
maximum number of operations permitted in a transaction

etcdmain/config.go Outdated
@@ -138,6 +138,7 @@ func newConfig() *config {
fs.UintVar(&cfg.TickMs, "heartbeat-interval", cfg.TickMs, "Time (in milliseconds) of a heartbeat interval.")
fs.UintVar(&cfg.ElectionMs, "election-timeout", cfg.ElectionMs, "Time (in milliseconds) for an election to timeout.")
fs.Int64Var(&cfg.QuotaBackendBytes, "quota-backend-bytes", cfg.QuotaBackendBytes, "Raise alarms when backend size exceeds the given quota. 0 means use the default quota.")
fs.UintVar(&cfg.MaxTxnOps, "max-txn-ops", cfg.MaxTxnOps, "Maximum operations per txn that etcd server allows; defaults to 128.")
Copy link
Contributor

Choose a reason for hiding this comment

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

the default should already show up in --help, don't bother restating here

Maximum number of operations permitted in a transaction.

@fanminshi fanminshi force-pushed the make_maxOpsPerTxn_configurable branch from 34b8bad to e9f464d Compare May 24, 2017 21:48
@fanminshi
Copy link
Member Author

All fixed.

@fanminshi fanminshi merged commit b003734 into etcd-io:master May 25, 2017
@gyuho gyuho removed this from the v3.3.0 milestone May 25, 2017
@gyuho gyuho added this to the v3.3.0 milestone May 25, 2017
@heyitsanthony heyitsanthony mentioned this pull request Jun 12, 2017
Closed
@purpleidea purpleidea mentioned this pull request Jun 13, 2017
Closed
@jonsyu1 jonsyu1 mentioned this pull request Jul 6, 2017
Merged
@hexfusion hexfusion mentioned this pull request Aug 31, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heyitsanthony heyitsanthony heyitsanthony approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fanminshi @heyitsanthony @gyuho