Use the common etcd client in etos-iut #83
Conversation
andmat900
requested review from
t-persson and
fredjn
and removed request for
a team
| if p == nil { | ||
| _, err := etcd.client.Delete(etcd.ctx, key) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("Failed to delete key %s: %s", key, err.Error()) | ||
| } | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
Why is this added? and why in the write function?
There was a problem hiding this comment.
This client shall implement the io.ReadWriter interface. The ETOS API applications are designed in a way that a database client shall be easily swapped with any other client which implements this common interface.
There was a problem hiding this comment.
I think the question is why are we adding delete in the write function. A question I also have
There was a problem hiding this comment.
If all potential database clients are supposed to conform to io.ReadWriter interface and be swappable, we have no other choice.
The io.ReadWriter interface seems to be more-suitable for a storage like a binary file, but here it requires workarounds to fit it into a key-value pair storage like etcd.
There was a problem hiding this comment.
I would rather we write a null value into the database rather than having this function do things that it shouldn't do.
If deletion is required then we should have a separate interface for that and in the Stop function cast the database to that interface to see if it is possible to delete.
client := h.database.Open(r.Context(), identifier)
c, canDelete := client.(database.Deleter)
if canDelete {
err := c.Delete()
} else {
_, err = client.Write(nil)
}
| logger := h.logger.WithField("identifier", identifier).WithContext(r.Context()) | ||
|
|
||
| id, err := uuid.Parse(r.URL.Query().Get("id")) | ||
| client := h.database.Open(r.Context(), identifier) | ||
|
|
||
| data := make([]byte, 4096) |
There was a problem hiding this comment.
Is 4096 always goign to be enough here?
There was a problem hiding this comment.
Normally it is around 130-150 bytes.
There was a problem hiding this comment.
make([]byte) should be fine here since we cannot know the size
There was a problem hiding this comment.
make([]byte) will not compile. It has to be a list of bytes that is larger than the actual data.
There was a problem hiding this comment.
Then I would recommend using something io.ReadAll instead, which will allocate a small size and grow it when necessary.
I.e. data, err := io.ReadAll(client)
| } | ||
| if len(dbResp.Kvs) == 0 { | ||
| err = fmt.Errorf("No key found: %s", key) | ||
| logger.Errorf("Failed to look up status request id: %s, %s", identifier, err.Error()) |
There was a problem hiding this comment.
Are we really doing a lookup here?
There was a problem hiding this comment.
In line 190 we do a look up by reading from the database.
Co-authored-by: Fredrik Fristedt <[email protected]>
| logger := h.logger.WithField("identifier", identifier).WithContext(r.Context()) | ||
|
|
||
| id, err := uuid.Parse(r.URL.Query().Get("id")) | ||
| client := h.database.Open(r.Context(), identifier) | ||
|
|
||
| data := make([]byte, 4096) |
There was a problem hiding this comment.
make([]byte) should be fine here since we cannot know the size
Applicable Issues
Description of the Change
This change migrates the generic ETOS IUT Provider to the common etcd client of ETOS API.
Alternate Designs
Possible Drawbacks
Sign-off
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Signed-off-by: Andrei Matveyeu, [email protected]