Reduce memory usage by fixing alignment of fields in structs#167
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
ostinru
left a comment
ostinru
left a comment
There was a problem hiding this comment.
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. |
| 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.
| InitiatedBy string `json:"initiated_by"` | ||
| InitiatedAt time.Time `json:"initiated_at"` | ||
| 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.
| 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.
| FailoverDelay time.Duration `config:"failover_delay" yaml:"failover_delay"` | ||
| KeepSuperWritableOnCriticalDiskUsage bool `config:"keep_super_writable_on_critical_disk_usage" yaml:"keep_super_writable_on_critical_disk_usage"` | ||
| 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.
| 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.
| ReadMasterLogPos int64 `db:"Read_Master_Log_Pos"` | ||
| 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 :(
| 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 :\
| - 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.
3 participants
No description provided.