add schema entities and mappings to process set node operational status protobuf message#19
Conversation
…us protobuf message
|
|
||
| type DistributionNodeOperationalStatusSetEvent { | ||
| "Distribution bucket operator" | ||
| bucketOperator: DistributionBucketOperator |
There was a problem hiding this comment.
I think this field should be required?
| processNodeLocationMetadata(operatorMetadata, metadataUpdate.location) | ||
| } | ||
| if (isSet(metadataUpdate.operationalStatus)) { | ||
| processNodeOperationalStatusMetadata('worker', undefined, metadataUpdate.operationalStatus) |
There was a problem hiding this comment.
Don't we need to pass the current status as the second argument so worker is not able to override the lead?
There was a problem hiding this comment.
Good catch. Addressed
| processNodeLocationMetadata(operatorMetadata, metadataUpdate.location) | ||
| } | ||
| if (isSet(metadataUpdate.operationalStatus)) { | ||
| processNodeOperationalStatusMetadata('worker', undefined, metadataUpdate.operationalStatus) |
| .getById(`${workingGroup}-${meta.workerId}`) | ||
|
|
||
| if (!maybeWorker) { | ||
| return invalidMetadata( |
There was a problem hiding this comment.
We return invalidMetadata here but processNodeOperationalStatusMetadata in mappings/storage/metadata.ts only calls it, should we be consistent about it?
There was a problem hiding this comment.
I think the return is just to break the function, alternatively we could just do
invalidMetadata()
return | `The storage bucket ${bucketId} is not active` | ||
| ) | ||
| // If the actor is a worker, check if the worker is the operator of the storage bucket | ||
| } else if (actor && storageBucket.operatorStatus.workerId !== actor.runtimeId) { |
There was a problem hiding this comment.
If I understand correctly, this check will ensure that regular worker can only update status of their own bucket and not somebody's elses. Should we also add a check for when the lead is the actor that ensures the meta.bucketId and meta.workerId matches storageBucket.operatorStatus.workerId?
There was a problem hiding this comment.
Correct, there is no validation currently to check meta.workerId = bucket.operatorStatus.workerId. So the drawback of the lack of this validation is that in the case of lead remark the operation would be performed even though meta.bucketId and meta.workerId do not match for any bucket, so, mappings are just looking for any valid meta.bucketId and set the status on that, (Simply ignoring what is the value of meta.workerId). And this is fine for the Lead context I think.
| const metadataEntity = | ||
| (await overlay.getRepository(StorageBucketOperatorMetadata).getById(bucketId)) || | ||
| overlay | ||
| .getRepository(StorageBucketOperatorMetadata) | ||
| .new({ id: bucketId, storageBucketId: bucketId }) |
There was a problem hiding this comment.
Just to confirm, .new() call is sync and doesn't need await?
There was a problem hiding this comment.
Correct, it just creates in-memory objects, which are scheduled to be saved on DB later.
| const operationalStatusSetEvent = new StorageNodeOperationalStatusSetEvent({ | ||
| ...genericEventFields(overlay, block, indexInBlock, extrinsicHash), | ||
| storageBucket: storageBucket.id, | ||
| operationalStatus: metadataEntity.nodeOperationalStatus || undefined, |
There was a problem hiding this comment.
Here we are emitting metadataEntity.nodeOperationalStatus, will this be already updated value after call to processNodeOperationalStatusMetadata?
There was a problem hiding this comment.
It will be set on the line.
metadataEntity.nodeOperationalStatus = processNodeOperationalStatusMetadata(But I just noticed that this StorageNodeOperationalStatusSetEvent evet is being emitted regardless of the outcome of the processNodeOperationalStatusMetadata.
So, I have changed the implementation only to emit the event when the status changes.
| SetNodeOperationalStatus, | ||
| `The distribution bucket operator ${distributionOperatorId} does not exist` | ||
| ) | ||
| } else if (actor && operator.workerId !== actor.runtimeId) { |
There was a problem hiding this comment.
Same question about the condition as above for storage
zeeshanakram3
force-pushed
the
process_set_node_operational_status_protobuf_message
branch
from
d9716de to
692abf5
Compare
|
@zeeshanakram3 |
2 participants
required for Joystream/joystream#4765