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

add raftlogdir commandline option #1110

Merged
merged 12 commits into from
Jun 14, 2021
Merged

Conversation

melihbirim
Copy link
Contributor

Hello Everyone, this PR contains a cli option to externalize raft log dirs.

Command-line option(--raftlogdir) to geth in order to move quorum-raft-state, raft-wal and raft-snap folders due to having large size.

This commit related with #1002

@jpmsam jpmsam requested review from prd-fox and vsmk98 January 11, 2021 12:27
Copy link
Contributor

@prd-fox prd-fox left a comment

Choose a reason for hiding this comment

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

Using the new flag works nicely. Just some comments around keeping existing functionality & code improvements.

cmd/utils/flags.go Outdated Show resolved Hide resolved
raft/backend.go Outdated Show resolved Hide resolved
raft/handler.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
prd-fox
prd-fox previously approved these changes Jan 13, 2021
@prd-fox
Copy link
Contributor

prd-fox commented Jan 14, 2021

Just needs you to run go fmt cmd/utils/flags.go to format the file and pass the linter.

prd-fox
prd-fox previously approved these changes Jan 15, 2021
@ricardolyn
Copy link
Contributor

@melihbirim can you update the PR with the latest master to solve the conflicts? which we could include in the future

@melihbirim
Copy link
Contributor Author

@melihbirim can you update the PR with the latest master to solve the conflicts? which we could include in the future

Hello, @ricardolyn I resolved the conflicts and updated my branch. Would you please check the PR. Thanks.

@ricardolyn
Copy link
Contributor

@melihbirim some unit tests are failing. also, it needs to be updated with the latest master changes.

We are still interested in merging this work 👍

@hemreari
Copy link
Contributor

Hi @ricardolyn, we have updated with the latest master branch and all unit tests are OK.

Copy link
Contributor

@ricardolyn ricardolyn left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@ricardolyn ricardolyn merged commit c1d6176 into Consensys:master Jun 14, 2021
@ricardolyn ricardolyn added the doc-change-required Indicates an issue or PR that requires doc to be updated label Jun 18, 2021
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Mar 22, 2022
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.

None yet

5 participants