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

pump/: Add syn-log option #509

Merged
merged 4 commits into from
Mar 29, 2019
Merged

pump/: Add syn-log option #509

merged 4 commits into from
Mar 29, 2019

Conversation

july2993
Copy link
Contributor

What problem does this PR solve?

add syn-log config option see pump.toml change

What is changed and how it works?

add syn-log config
change template compaction-total-size-multiplier from 8 -> 8.0 which should be float

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    check config work by log

Code changes

Side effects

Related changes

@july2993
Copy link
Contributor Author

@GregoryIan PTAL

pump/server.go Outdated Show resolved Hide resolved
pump/server.go Outdated Show resolved Hide resolved
pump/storage/storage.go Outdated Show resolved Hide resolved
@july2993
Copy link
Contributor Author

/run-all-tests

@IANTHEREAL
Copy link
Collaborator

LGTM

@july2993
Copy link
Contributor Author

/run-unit-test

@@ -64,7 +64,7 @@ type Config struct {
configFile string
printVersion bool
tls *tls.Config
Storage *storage.Config `toml:"storage" json:"storage"`
Storage storage.Config `toml:"storage" json:"storage"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we get when this is changed from pointer to value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just no need to check if Storage is nil (when no Storage** is config in file)

}

// GetSyncLog return sync-log config option
func (c *Config) GetSyncLog() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also handle the nil value in options.WithSync so that:

  1. the other callers also don't have to worry about nils
  2. we can use config.SyncLog like when we are accessing the other config values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithSync accept bool not *bool, and if accept *bool if pass nil seems meaningless.

@suzaku
Copy link
Contributor

suzaku commented Mar 29, 2019

LGTM

@IANTHEREAL IANTHEREAL merged commit fae74ec into master Mar 29, 2019
@IANTHEREAL IANTHEREAL deleted the hjh/syn-log branch March 29, 2019 13:40
july2993 added a commit that referenced this pull request Apr 10, 2019
july2993 added a commit that referenced this pull request Apr 10, 2019
* pump: Extract function and add unit tests (#503)

* pump/: Add syn-log option (#509)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants