filetable: fix NPE when an empty file is downloaded from userfile #63467
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @pbardea)
a discussion (no related file):
Looks reasonable to me. It might be nice to write a few words in the commit message about why we aren't using the size field in the metadata table.
pkg/cli/userfiletable_test.go, line 175 at r1 (raw file):
}) t.Run("get", func(t *testing.T) {
tc.name+"_get" might make for clearer output
pkg/cli/userfiletable_test.go, line 181 at r1 (raw file):
cliOutput, err := c.RunWithCaptureArgs(cmd) require.NoError(t, err) if strings.Contains(cliOutput, "ERROR: no files matched requested path or path pattern") {
Would it be worth it to be less strict here? Given the line below, I imagine anything starting with ERROR would be unexpected.
It's a little unfortunate that RunWithCaptureArgs swallows the error produced by what I hope would be the non-zero exit of the command.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/cloudimpl/filetable/file_table_read_writer.go, line 701 at r1 (raw file):
fileID = vals[0].([]byte) if vals[1] != nil { sz = vals[1].(int64)
perhaps we should use tree.MustBeX methods instead of direct cases?
There was a problem hiding this comment.
Reviewable status:
pkg/cli/userfiletable_test.go, line 175 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
tc.name+"_get"might make for clearer output
As a bonus, it looks like these other test cases would be good candidates for subtests of the tc.name one above, which would help avoid the awkward tc.name+subTestName formulation for all of these subtests.
This change ensures that a file with a metadata entry but no payload does not throw an exception when attempting to compute the size of the file (which will be zero). Fixes: cockroachdb#62605 Release note: None
adityamaru
force-pushed
the
userfile-size-npe
branch
from
a2afb06 to
c544874
Compare
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, stevendanna (Steven Danna) wrote…
Looks reasonable to me. It might be nice to write a few words in the commit message about why we aren't using the
sizefield in the metadata table.
I wanted to minimize the diff so that we can consider a backport for this, but apart from that I don't really see a reason we can't consult the file size we write in the metadata row. I guess there is an argument that this is the most accurate source of truth, but if the former were to go out of sync we probably have another bug.
pkg/cli/userfiletable_test.go, line 175 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
As a bonus, it looks like these other test cases would be good candidates for subtests of the
tc.nameone above, which would help avoid the awkward tc.name+subTestName formulation for all of these subtests.
Done.
pkg/cli/userfiletable_test.go, line 181 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Would it be worth it to be less strict here? Given the line below, I imagine anything starting with
ERRORwould be unexpected.It's a little unfortunate that RunWithCaptureArgs swallows the error produced by what I hope would be the non-zero exit of the command.
Done.
pkg/storage/cloudimpl/filetable/file_table_read_writer.go, line 701 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
perhaps we should use tree.MustBeX methods instead of direct cases?
The internal executor above can use these methods because the results of the iterator are Datums, but for a SQLConnExecutor the results are driver.Value which I believe can't be converted to datums easily.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status:
|
TFTR! |
|
Build succeeded: |
|
@adityamaru friendly ping on this: does this still need to be backported? |
Fixes: #62605
Release note: None