ZanderXu commented on PR #4872: URL: https://github.com/apache/hadoop/pull/4872#issuecomment-1323677572
> Sorry for being late to the party here. The changes to `getAdditionalBlock` look fine to me -- we already check for `WRITE` operation later in the same method, this just moves the check a little earlier. No issues there. > > The changes to `getAdditionalDatanode`, which legitimately seems not to do any write modification to the namespace (e.g. no write lock is held), are less obviously okay. I think the real problem here comes from the mismatch: `FSNamesystem#getAdditionalDatanode` considers itself a read-op, but in `ClientProtocol`, it is not annotated as `@ReadOnly`, so it's considered like a write op. So we can _either_ change it to `OperationCategory.WRITE`, as proposed in this PR, _or_ we can mark it as `@ReadOnly(isCoordinated = true)`. Either one would solve the current problem. Marking it read-only is better from a perf/scalability perspective if it really can be safely served by an ObserverNode. Looking through `getAdditionalDatanode`, the only part I would be worried about is that we generate a new block token as part of the response. AFAICT from the handling of the keys used by `BlockTokenSecretManager`, the DataNodes will fetch/trust keys produced by all NNs, not just the active, so I th ink this is all good. > > So, unless anyone sees a reason why we _can't_ mark `getAdditionalDatanode` as `@ReadOnly`, I would propose to do that instead of switching its `OperationCategory` to `WRITE`. @xkrogen Sir, thanks for your carefully review and nice suggestion. NameNode will choose one new datanode with considering datanode state and reserved capacity during handling the `getAdditionalDatanode`, such as stale datanode, busy datanode, slow datanode, maintenance datanode, etc.. So I think `getAdditionalDatanode` should be handled by `ActiveNameNode`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
