Add function to create WAL files #18605

lucasrod16 · 2024-09-19T06:14:38Z

This PR adds a new function to consolidate WAL file creation into a single function to help reduce errors and make the process more consistent.

k8s-ci-robot · 2024-09-19T06:14:47Z

Hi @lucasrod16. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov-commenter · 2024-09-19T06:32:22Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68.76%. Comparing base (59cfd7a) to head (b2e1f60).

❗ Current head b2e1f60 differs from pull request most recent head 680eadf

Please upload reports for the commit 680eadf to get more accurate results.

Files with missing lines	Patch %	Lines
server/storage/wal/wal.go	71.42%	2 Missing and 2 partials ⚠️
server/storage/wal/file_pipeline.go	0.00%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/storage/wal/repair.go	`56.14% <100.00%> (ø)`
server/storage/wal/file_pipeline.go	`90.69% <0.00%> (ø)`
server/storage/wal/wal.go	`57.73% <71.42%> (+0.27%)`	⬆️

... and 20 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18605      +/-   ##
==========================================
- Coverage   68.79%   68.76%   -0.04%     
==========================================
  Files         420      420              
  Lines       35522    35535      +13     
==========================================
- Hits        24438    24434       -4     
- Misses       9657     9669      +12     
- Partials     1427     1432       +5

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59cfd7a...680eadf. Read the comment docs.

ahrtr · 2024-09-22T19:05:46Z

server/storage/wal/wal_test.go

+		{
+			name:     "creating standard file should succeed",
+			fileType: &os.File{},
+		},
+		{
+			name:     "creating locked file should succeed",
+			fileType: &fileutil.LockedFile{},
+		},
+		{
+			name:     "creating standard file with truncate should succeed",
+			fileType: &os.File{},
+			truncate: true,
+		},
+		{
+			name:     "creating locked file with truncate should succeed",
+			fileType: &fileutil.LockedFile{},
+			truncate: true,
+		},


can you also add two cases to verify the following two cases?

truncate:true, it should truncate the file content if present

truncate: false: it shouldn't truncate the file content if present

Added test cases to verify truncate behavior

ahrtr · 2024-09-22T19:07:58Z

server/storage/wal/wal.go

+// To create a locked file, use *fileutil.LockedFile type parameter.
+// To create a standard file, use *os.File type parameter.
+// If truncate is true, the file will be truncated if it already exists.
+func createNewWALFile[T *os.File | *fileutil.LockedFile](path string, truncate bool) (T, error) {


truncate is imperative name, while forceNew is declarative name.

Suggested change

func createNewWALFile[T *os.File | *fileutil.LockedFile](path string, truncate bool) (T, error) {

func createNewWALFile[T *os.File | *fileutil.LockedFile](path string, forceNew bool) (T, error) {

Updated to use declarative name forceNew

ahrtr · 2024-09-22T19:08:16Z

/ok-to-test

serathius

Awesome work, nice use of generics.

ahrtr · 2024-09-24T08:44:48Z

server/storage/wal/wal_test.go

+			case *os.File:
+				f, err = createNewWALFile[*os.File](p, tt.forceNew)
+				require.IsType(t, &os.File{}, f)
+			case *fileutil.LockedFile:
+				f, err = createNewWALFile[*fileutil.LockedFile](p, tt.forceNew)
+				require.IsType(t, &fileutil.LockedFile{}, f)


Suggested change

case *os.File:

f, err = createNewWALFile[*os.File](p, tt.forceNew)

require.IsType(t, &os.File{}, f)

case *fileutil.LockedFile:

f, err = createNewWALFile[*fileutil.LockedFile](p, tt.forceNew)

require.IsType(t, &fileutil.LockedFile{}, f)

case *os.File:

f, err = createNewWALFile[*os.File](p, tt.forceNew)

require.IsType(t, &os.File{}, f)

case *fileutil.LockedFile:

f, err = createNewWALFile[*fileutil.LockedFile](p, tt.forceNew)

require.IsType(t, &fileutil.LockedFile{}, f)

default:

panic("unknown file type")

Added a default case to handle unknown file types. Good catch on handling that scenario

Signed-off-by: Lucas Rodriguez <lucas.rodriguez9616@gmail.com>

ahrtr

LGTM

Nice work!

k8s-ci-robot · 2024-09-24T18:44:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, lucasrod16, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr,serathius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test size/L labels Sep 19, 2024

lucasrod16 force-pushed the use-single-function-to-create-WAL-files branch 2 times, most recently from c2852b2 to d0cd63a Compare September 19, 2024 18:12

k8s-ci-robot added size/M and removed size/L labels Sep 19, 2024

ahrtr reviewed Sep 22, 2024

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Sep 22, 2024

lucasrod16 force-pushed the use-single-function-to-create-WAL-files branch from d0cd63a to 61989a0 Compare September 24, 2024 02:18

serathius approved these changes Sep 24, 2024

View reviewed changes

k8s-ci-robot added the approved label Sep 24, 2024

ahrtr reviewed Sep 24, 2024

View reviewed changes

Add function to create WAL files

680eadf

Signed-off-by: Lucas Rodriguez <lucas.rodriguez9616@gmail.com>

lucasrod16 force-pushed the use-single-function-to-create-WAL-files branch from 61989a0 to 680eadf Compare September 24, 2024 17:55

k8s-ci-robot added size/L and removed size/M labels Sep 24, 2024

ahrtr approved these changes Sep 24, 2024

View reviewed changes

ahrtr merged commit 5704c61 into etcd-io:main Sep 24, 2024
38 checks passed

lucasrod16 deleted the use-single-function-to-create-WAL-files branch September 24, 2024 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function to create WAL files #18605

Add function to create WAL files #18605

lucasrod16 commented Sep 19, 2024

k8s-ci-robot commented Sep 19, 2024

codecov-commenter commented Sep 19, 2024 •

edited

Loading

ahrtr Sep 22, 2024

lucasrod16 Sep 24, 2024

ahrtr Sep 22, 2024

lucasrod16 Sep 24, 2024

ahrtr commented Sep 22, 2024

serathius left a comment

ahrtr Sep 24, 2024

lucasrod16 Sep 24, 2024

ahrtr left a comment

k8s-ci-robot commented Sep 24, 2024

	func createNewWALFile[T os.File \| fileutil.LockedFile](path string, truncate bool) (T, error) {
	func createNewWALFile[T os.File \| fileutil.LockedFile](path string, forceNew bool) (T, error) {

Add function to create WAL files #18605

Add function to create WAL files #18605

Conversation

lucasrod16 commented Sep 19, 2024

k8s-ci-robot commented Sep 19, 2024

codecov-commenter commented Sep 19, 2024 • edited Loading

Codecov Report

ahrtr Sep 22, 2024

Choose a reason for hiding this comment

lucasrod16 Sep 24, 2024

Choose a reason for hiding this comment

ahrtr Sep 22, 2024

Choose a reason for hiding this comment

lucasrod16 Sep 24, 2024

Choose a reason for hiding this comment

ahrtr commented Sep 22, 2024

serathius left a comment

Choose a reason for hiding this comment

ahrtr Sep 24, 2024

Choose a reason for hiding this comment

lucasrod16 Sep 24, 2024

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 24, 2024

codecov-commenter commented Sep 19, 2024 •

edited

Loading