-
Notifications
You must be signed in to change notification settings - Fork 18
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
Reduce memory usage by fixing alignment of fields in structs #167
base: master
Are you sure you want to change the base?
Conversation
701a345
to
6afcc00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR reduces readablity.
Leave source code to humans, not computers.
Could you point at exact locations where readability is reduced? I don't see any such place. |
LogLevel string `config:"loglevel"` | ||
Commands map[string]string `config:"commands"` | ||
Queries map[string]string `config:"queries"` | ||
MySQL MySQLConfig `config:"mysql"` | ||
Log string `config:"log"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Log
and LogLevel
shouldn't be split.
StartedBy string `json:"started_by"` | ||
StartedAt time.Time `json:"started_at"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that we would like to split fields with the same prefix.
ReplicationPort int `config:"replication_port" yaml:"replication_port"` | ||
ReplicationPassword string `config:"replication_password,required" yaml:"replication_password"` | ||
PidFile string `config:"pid_file" yaml:"pid_file"` | ||
Password string `config:"password,required"` | ||
ReplicationSslCA string `config:"replication_ssl_ca" yaml:"replication_ssl_ca"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this type has logic structure:
- User definition: user, password, port
- Replication user definition: user, password, port (I suppose we can exchange
ReplicationPort
withReplicationPassword
to have consistent order), and options. - Other fields.
SemiSyncEnableLag int64 `config:"semi_sync_enable_lag" yaml:"semi_sync_enable_lag"` | ||
Failover bool `config:"failover" yaml:"failover"` | ||
FailoverCooldown time.Duration `config:"failover_cooldown" yaml:"failover_cooldown"` | ||
FailoverDelay time.Duration `config:"failover_delay" yaml:"failover_delay"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't split options with prefix Failover
.
ASync bool `config:"async" yaml:"async"` | ||
AsyncAllowedLag time.Duration `config:"async_allowed_lag" yaml:"async_allowed_lag"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe ASync
and AsyncAllowedLag
shouldn't be split either.
ReplMonTableName string `config:"repl_mon_table_name" yaml:"repl_mon_table_name"` | ||
ReplMonWriteInterval time.Duration `config:"repl_mon_write_interval" yaml:"repl_mon_write_interval"` | ||
ReplMonErrorWaitInterval time.Duration `config:"repl_mon_error_wait_interval" yaml:"repl_mon_error_wait_interval"` | ||
ReplMonSlaveWaitInterval time.Duration `config:"repl_mon_slave_wait_interval" yaml:"repl_mon_slave_wait_interval"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we would like to split ReplMon
options. It is convenient to have them in one place.
UseSSL bool `config:"use_ssl" yaml:"use_ssl"` | ||
KeyFile string `config:"keyfile" yaml:"keyfile"` | ||
CertFile string `config:"certfile" yaml:"certfile"` | ||
CACert string `config:"ca_cert" yaml:"ca_cert"` | ||
VerifyCerts bool `config:"verify_certs" yaml:"verify_certs"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this section (Auth
- VerifyCerts
) is logically structured to be the authentication section of the ZookeeperConfig
.
LastIOErrno int `db:"Last_IO_Errno"` | ||
LastIOError string `db:"Last_IO_Error"` | ||
LastSQLErrno int `db:"Last_SQL_Errno"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is convenient to place all errors in one location.
P.S. It seems we forgot to place LastError
with the other Last
errors :(
} | ||
|
||
// ReplicaStatusStruct contains SHOW REPLICA STATUS response | ||
type ReplicaStatusStruct struct { | ||
SourceHost string `db:"Source_Host"` | ||
SourcePort int `db:"Source_Port"` | ||
LastIOError string `db:"Last_IO_Error"` | ||
SourceLogFile string `db:"Source_Log_File"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we shouldn't split fields with the same prefix.
P.S. There is the same about LastError
field :\
@@ -44,6 +44,9 @@ linters-settings: | |||
- name: empty-block | |||
- name: unreachable-code | |||
- name: redefines-builtin-id | |||
govet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging the PR, we will lose the ability to rearrange fields in structures :(
In my opinion, it is not good for readability.
No description provided.