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

Reduce memory usage by fixing alignment of fields in structs #167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

secwall
Copy link
Contributor

@secwall secwall commented Mar 5, 2025

No description provided.

@secwall secwall force-pushed the align-fields-in-structs branch from 701a345 to 6afcc00 Compare March 5, 2025 13:58
@secwall secwall changed the title Recude memory usage by fixing alignment of fields in structs Reduce memory usage by fixing alignment of fields in structs Mar 5, 2025
@secwall secwall requested a review from teem0n March 5, 2025 14:02
Copy link
Collaborator

@ostinru ostinru left a 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.

@secwall
Copy link
Contributor Author

secwall commented Mar 5, 2025

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"`
Copy link
Collaborator

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"`
Copy link
Collaborator

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"`
Copy link
Collaborator

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 with ReplicationPassword 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"`
Copy link
Collaborator

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"`
Copy link
Collaborator

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"`
Copy link
Collaborator

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"`
Copy link
Collaborator

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"`
Copy link
Collaborator

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"`
Copy link
Collaborator

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:
Copy link
Collaborator

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.

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

Successfully merging this pull request may close these issues.

3 participants