Implemented methods for checking if the NIL file exists and is valid and updating the NIL file.#66
Implemented methods for checking if the NIL file exists and is valid and updating the NIL file.#66evanSpendlove wants to merge 9 commits intomasterfrom
Conversation
evanSpendlove
requested review from
alicjakwie,
cianbgoog,
deniadelcid and
leczb
| for i := firstID; i <= lastID; i++ { | ||
| var id string | ||
| if i <= 9 { | ||
| id = fmt.Sprintf("0%d", i) |
There was a problem hiding this comment.
replace this with fmt.Spintf("%02d", i)
| return client | ||
| } | ||
|
|
||
| func TestDeleteRandomFile(t *testing.T) { |
There was a problem hiding this comment.
it'd be nice to test that we record some metrics (and maybe even that they're within some sane range). Fine to do this in a future PR though.
|
|
||
| metricpb "github.com/google/cloudprober/metrics/proto" | ||
| monitorpb "github.com/googleinterns/step224-2020/config/proto" | ||
| m "github.com/googleinterns/step224-2020/hermes/probe/metrics" |
| // - name: pass the name for this probe instance. | ||
| // Returns: | ||
| // - hermesExtension: returns the HermesProbeDef extension. | ||
| func genTestConfig(name string) *monitorpb.HermesProbeDef { |
There was a problem hiding this comment.
Nit: I would call this genHermesProbeDef.
| "github.com/google/cloudprober/logger" | ||
| "github.com/googleapis/google-cloud-go-testing/storage/stiface" | ||
| "github.com/googleinterns/step224-2020/hermes/probe" | ||
| "github.com/googleinterns/step224-2020/hermes/probe/mock" |
There was a problem hiding this comment.
This package should be called fakegcs (as it's not a mock).
| Name: "hermes", | ||
| TargetSystem: monitorpb.Target_GOOGLE_CLOUD_STORAGE, | ||
| TotalSpaceAllocatedMib: int64(100), | ||
| BucketName: bucketName, |
There was a problem hiding this comment.
Can you verify whether this test runs? It isn't clear were bucketName is being defined..
| const ( | ||
| firstID = int32(1) | ||
| lastID = int32(50) | ||
| contents = "abc123" | ||
| hash = "6367c48dd193d56ea7b0baad25b19455e529f5ee" | ||
| bucketName = "test_bucket_5" | ||
| ) |
There was a problem hiding this comment.
Oh. I see it now. Always, place your constants towards the beginning of the file, right below your imports.
There was a problem hiding this comment.
FYI, this is a good rule for exported constants. For non-exported consts that are only used in a single function, it's better to declare them within the function.
| testProbeName := "testDelete1" | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Can you add more test scenarios? Such as: deleting a file that's not present, deleting multiple file, and deleting the NIL file.
| probeErr = CheckNilFile(ctx, target, client, logger) | ||
| probeErr = CheckJournal(ctx, target, client, logger) | ||
| if probeErr.Status != m.Success { | ||
| t.Errorf("checkNilFile failed: %v", probeErr) |
| if err := RecordAPILatency(target.LatencyMetrics.APICallLatency, metrics.APICreateFile, func() *probe.ProbeError { | ||
| writer = client.Bucket(bucket).Object(file.name).NewWriter(ctx) | ||
| length, err := io.Copy(writer, file.reader()) | ||
| if writerErr := writer.Close(); writerErr != nil { |
There was a problem hiding this comment.
It's surprising that this is done before inspecting err.
I assume this is to ensure that writer.Close() is called in any case.
I would recommend using defer for this, even though this throws away the error. Rationale: it's unlikely that Close() returns an error. If something goes wrong, it's going to be Copy() that returns an error.
I would do
writer = client.Bucket(bucket).Object(file.name).NewWriter(ctx)
defer writer.Close()
| var writer stiface.Writer | ||
| if err := RecordAPILatency(target.LatencyMetrics.APICallLatency, metrics.APICreateFile, func() *probe.ProbeError { | ||
| writer = client.Bucket(bucket).Object(file.name).NewWriter(ctx) | ||
| length, err := io.Copy(writer, file.reader()) |
There was a problem hiding this comment.
length is somewhat misleading. I would call this bytesWritten or just n (which is the customary name for "count" variables).
| func findJournal(ctx context.Context, target *probe.Target, client stiface.Client, logger *logger.Logger) (string, *probe.ProbeError) { | ||
| bucket := target.Target.GetBucketName() | ||
| file := &journalFile{} | ||
| var fileName *string |
There was a problem hiding this comment.
This doesn't have to be a pointer, just string.
| return probe.NewProbeError(metrics.APICallFailed, fmt.Errorf("findJournal(): unable to list files: %w", err)) | ||
| } | ||
| file.name = obj.Name | ||
| *fileName = obj.Name |
There was a problem hiding this comment.
What if there are more than one results from the query?
| } | ||
|
|
||
| fileID, err := strconv.Atoi(journal.Intent.Filename[len("Hermes_"):len("Hermes_ID")]) | ||
| fileID, err := strconv.Atoi(journal.Intent.Filename[len(hermesPrefix):len(hermesIDPrefix)]) |
There was a problem hiding this comment.
What if the filename is corrupted and its length is shorter than hermesIDPrefix? Currently it would crash the process.
Add a unit test case to see if fail; fix the code; then run the test again to see it pass.
hermes/probe/journal/journal.go
Outdated
| func (j *journalFile) checksumContents() error { | ||
| h := sha1.New() | ||
| if _, err := io.Copy(h, j.reader()); err != nil { | ||
| return fmt.Errorf("verifyNilFile(): unable to compute checksum of file contents: %w", err) |
hermes/probe/journal/journal.go
Outdated
| func (j *journalFile) unmarshalContents() (*pb.StateJournal, *probe.ProbeError) { | ||
| journal := &pb.StateJournal{} | ||
| if err := proto.Unmarshal(j.contents, journal); err != nil { | ||
| return nil, probe.NewProbeError(metrics.FileCorrupted, fmt.Errorf("verifyNilFile(): unable to unmarshal nil file proto: %w", err)) |
hermes/probe/journal/journal.go
Outdated
| // Author: Evan Spendlove, GitHub: evanSpendlove | ||
|
|
||
| // Package journal implements the probe operation for checking if the Journal file | ||
| // on the target storage system is present. It also verifies the contetns of |
There was a problem hiding this comment.
nit
| // on the target storage system is present. It also verifies the contetns of | |
| // on the target storage system is present. It also verifies the contents of |
| return err | ||
| } | ||
|
|
||
| j := &journalFile{ |
There was a problem hiding this comment.
| j := &journalFile{ | |
| j := &journalFile { |
| bucket := target.Target.GetBucketName() | ||
| var fileName *string | ||
|
|
||
| err := RecordAPILatency(target.LatencyMetrics.APICallLatency, metrics.APIListFiles, func() *probe.ProbeError { |
There was a problem hiding this comment.
You don't seem do do anything with the err defined here. Can you add a check after this definition?
| logger.Debug("UpdateJournal(): in-memory journal successfully marshalled, proceeding to upload to target storage system.") | ||
|
|
||
| var writer stiface.Writer | ||
| if err := RecordAPILatency(target.LatencyMetrics.APICallLatency, metrics.APICreateFile, func() *probe.ProbeError { |
There was a problem hiding this comment.
To make this look a bit cleaner, can you move the assignment of err outside the if statement? Something like:
err := RecordAPILatency(...)
if err != nil {...}
|
|
||
| var reader stiface.Reader | ||
| err := RecordAPILatency(target.LatencyMetrics.APICallLatency, metrics.APIGetFile, func() *probe.ProbeError { | ||
| r, err := fileObject.NewReader(ctx) |
There was a problem hiding this comment.
Why not assign reader directly in here?
| const ( | ||
| firstID = int32(1) | ||
| lastID = int32(50) | ||
| contents = "abc123" | ||
| hash = "6367c48dd193d56ea7b0baad25b19455e529f5ee" | ||
| bucketName = "test_bucket_5" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Move constants towards the top of the file, below your imports.
4 participants
No description provided.