Reduce memory usage by fixing alignment of fields in structs #167
Conversation
secwall
force-pushed
the
align-fields-in-structs
branch
from
701a345 to
6afcc00
Compare
secwall
changed the title
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.
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.
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.
I believe this type has logic structure:
- User definition: user, password, port
- Replication user definition: user, password, port (I suppose we can exchange
ReplicationPortwithReplicationPasswordto 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.
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.
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.
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.
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.
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.
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.
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.