Replace backbeat client with cloudserver client #2679
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
from
6398439 to
269de9b
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (52.15%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files
... and 9 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.1 #2679 +/- ##
===================================================
- Coverage 74.81% 73.89% -0.92%
===================================================
Files 201 201
Lines 13579 13458 -121
===================================================
- Hits 10159 9945 -214
- Misses 3410 3503 +93
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
package.json
Outdated
| "vaultclient": "scality/vaultclient#8.5.1", | ||
| "werelogs": "scality/werelogs#8.2.1" | ||
| "werelogs": "scality/werelogs#8.2.1", | ||
| "@scality/cloudserverclient": "file:../cloudserverclient" |
conf/config.json
Outdated
| "replicaSet": "rs0", | ||
| "readPreference": "primary", | ||
| "database": "metadata" | ||
| "replicaSetHosts": "localhost:27017", |
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
5 times, most recently
from
d06be27 to
11d7667
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
2 times, most recently
from
130fc22 to
5c4933f
Compare
| const { attachReqUids } = require('../../../lib/clients/utils'); | ||
|
|
||
| const BackbeatTask = require('../../../lib/tasks/BackbeatTask'); | ||
| const { BatchDeleteCommand } = require('../../../lib/clients/smithy/build/smithy/source/typescript-codegen'); |
There was a problem hiding this comment.
Do not pay attention to these imports path. They will be updated once the new library is released
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
from
5c4933f to
616c43b
Compare
| return done(err); | ||
| } | ||
|
|
||
| const backbeatClient = this.getBackbeatClient(accountId); |
There was a problem hiding this comment.
Side note : There are a lot of variables, function names and possibly file names that would benefit from a rename, from backbeatClient to cloudserverClient etc.
I don't wanna do it now because it will add a lot of noise to that PR, and it's not even that straightforward to do with this silly programming language
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
from
616c43b to
dcd1111
Compare
| Key: destEntry.getObjectKey(), | ||
| CanonicalID: destEntry.getOwnerId(), | ||
| // TODO : missing content length | ||
| ContentLength: partSize, |
There was a problem hiding this comment.
I removed content-length. Smithy doesn't allow us to have a content length variable that we defined ourselves to be used as the header, because it's calculating the header itself.
Only thing I'm worried about is that the new ContentLength from Smithy might not be equal to the previous one : I think the new ones uses the whole request, when our old content length was partObj.getObjSize()
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
3 times, most recently
from
404c946 to
897ee3e
Compare
| // eslint-disable-next-line no-param-reassign | ||
| err.origin = 'target'; | ||
| if (err.ObjNotFound || err.code === 'ObjNotFound') { | ||
| if (err.ObjNotFound || err.code === 'ObjNotFound' || |
There was a problem hiding this comment.
This isn't super nice error checking 🫤
There was a problem hiding this comment.
we cannot guaarantee the type of error there? is it always an arsenal error, or an sdk one? if sdk you should be able to use instanceof, no?
Side note: can't it also be a NoSuchVersion in this case, if the versionID is not empty?
There was a problem hiding this comment.
I double checked, we can use err.name === 'ObjNotFound' (or err.code too)
There was a problem hiding this comment.
For the side note, I wasnt able to trigger any NoSuchVersion, but I was able to trigger NoSuchBucket.
In any case, nto very important imo : the error will always be thrown, this if statement is only there to not log the error in this specific situation
| .then(data => { | ||
| sourceEntry.setReplicationSiteDataStoreVersionId(this.site, | ||
| data.versionId); | ||
| // TODO : review metadata metrics |
There was a problem hiding this comment.
@francoisferrand I think using JSON.stringify will work, I see that the function .getSerialized() in Arsenal also returns a string from JSON.stringify that is then used in this same _publishMetadataWriteMetrics, but I'm not sure that "command.input" is equivalent to the old "destReq.httpRequest.body" data we used to call it with
There was a problem hiding this comment.
There are a few occurences where this _publishMetadataWriteMetrics is an issue for me.
Actually, we can add a middleware to extract the request body like this, and pass it to the metrics just as before.
Although, I have found that here it would likely be useless, because as you can see in the command above, there is Body in the command, so the length would always be 0.
I have only found one occurence where we the request has a body
command.middlewareStack.add(
next => async args => {
const request = args.request as any;
console.log("content-length:", request?.headers?.['content-length'])
console.log("len", Buffer.byteLength(request.body as any))
return next(args);
},
{ step: 'finalizeRequest' }
);
There was a problem hiding this comment.
One more thought : With the old SDK, we were using destReq.httpRequest.body, but if you look at the command, there is no proper body parameter, and with the new v3 client, I tried to get the body with the middleware and it is undefined...
Anyways, probably not worth it to overthink this problem, we just need to clearly state what is the metric we want to compute first (I believe that here it's the Tags, since the api is putTagging), so we probably just want to do something like
this._publishMetadataWriteMetrics(JSON.stringify(command.input.Tags), writeStartTime)
| if (err) { | ||
| return this._ringReader.send(command) | ||
| .then(data => { | ||
| // TODO : Review : data[0] doesn't make sense, considering the output shape.. : |
There was a problem hiding this comment.
Need input from other ppl who know this stuff.
Considering the output of getRaftId is a string,
Either,
- We indeed want to read the first character of that string
- Or we expect it to be an array and want to read the first element, but in that case there is a problem with api definition
Either way, weird api design
There was a problem hiding this comment.
The output looks like this, but can RaftID be multiple digit ?
There was a problem hiding this comment.
Yes each raft session has an id, so if you have 10 then it'll be 2 digits
For instance in Scuba we type it like htis: https://github.com/scality/scuba/blob/d6c285163fb7fa2113aecc32f14623bda60e7508/lib/utils/logId.ts#L6
There was a problem hiding this comment.
Mhh, so if raftId == '13', sounds like the old code getting the raftId from data[0] would be getting '1' instead of '13' 🤔
There was a problem hiding this comment.
maybe @scality/hopocalypse team can help better understand this, i.e. how the API behaves and maybe why we used to take only the first character of the string?
There was a problem hiding this comment.
This is not used in S3C, we don't have an IngestionPopulator.
Here is our list of entrypoints for backbeat:
- https://github.com/scality/Federation/blob/development/9.5/roles/run-backbeat/templates/supervisord.conf.j2
- https://github.com/scality/Federation/blob/development/9.5/roles/run-bucket-notifications/templates/supervisord.conf.j2
No bin/ingestion.js in there
| ], done); | ||
| } | ||
|
|
||
| // TODO : review streaming logic.. |
There was a problem hiding this comment.
TODO : review
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
from
897ee3e to
3f6b0a3
Compare
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
11 times, most recently
from
5eef1b8 to
6121867
Compare
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
from
6121867 to
26686e8
Compare
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
11 times, most recently
from
e9bb058 to
1900121
Compare
SylvainSenechal
force-pushed
the
improvement/BB-706
branch
from
1900121 to
2ad58ce
Compare
WIP
Issue: BB-706