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

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

Merged
merged 2 commits into from
May 25, 2017

Conversation

fanminshi
Copy link
Member

Fixes #7826

@@ -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.

@@ -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

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
--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.

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

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

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants