vyommani commented on PR #1112:
URL: https://github.com/apache/ranger/pull/1112#issuecomment-5150345839
> @vyommani - how about adding `normalize()` method in `RangerServiceDef`
which will perform the operation in thread-safe way. This will isolate the
details within `RangerServiceDef`.
Good suggestion, and it's true that today's
ServiceDefUtil.normalizeDataMaskDef()/normalizeRowFilterDef() are technically
callable directly on an un-copied instance, which is a real gap worth closing
eventually. One clarification on the "thread-safe" framing: the current fix
doesn't need any synchronization at all - normalize() returns a fresh copy with
no shared mutable state, so there's nothing left to lock. Moving it onto
RangerServiceDef wouldn't change that; it'd still be copy-then-mutate, just
called as serviceDef.normalize() instead of
ServiceDefUtil.normalize(serviceDef).
We'd rather not fold this into the current PR. Making it worthwhile (not
just a thin wrapper) means normalizeDataMaskDef()/normalizeRowFilterDef()
becoming private instance methods only reachable through the new normalize(),
which means re-touching every call site we just finished auditing and testing ,
plus a decision on whether
normalizeAccessTypeDefs()/getMarkerAccessTypes()/getDataMaskType() move too
(leaving them as static utilities would create a half-migrated, inconsistent
API). None of that is hard, but it's real scope on a patch that just went green.
We'll file this as a follow-up and take a pass at moving the whole
normalize-family onto the model class in one coherent change, rather than
partially migrating it here.
> @vyommani - instead of taking this fix and then subsequent patch to move
normalization to RangerServiceDef, why not skip the current fix and directly go
with the final/desired fix?
Update on the normalize()-as-instance-method idea - started the migration on
a separate branch and found something worth flagging before we go further.
normalizeDataMaskDef()/normalizeRowFilterDef() call
ServiceDefUtil.mergeResourceDef()/mergeAccessTypeDef(), which are private
static on ServiceDefUtil. These aren't trivial - they're real merge logic
(field-by-field precedence rules, a couple of special-cased conditions, manual
per-key map merging). Moving the normalize family onto RangerServiceDef means
either making these private helpers public (leaking internal merge logic as
permanent public API for one caller) or cascading the move further into
RangerServiceDef. Neither is great RangerServiceDef is a plain data model
everywhere else in the codebase, and this would be the first real business
logic living on it.
Given that design cost, I don't think the full migration is worth pursuing
as originally scoped. If we still want the nicer call syntax, a thin delegating
wrapper gets it for free with none of the downside:
public RangerServiceDef normalize() {
return ServiceDefUtil.normalize(this);
}
Everything else - normalizeDataMaskDef, normalizeRowFilterDef,
mergeResourceDef, mergeAccessTypeDef - stays exactly where it is, exactly as
private as it is today. Zero cascade, zero visibility changes, zero risk.
Open to your read on whether even the thin wrapper is worth adding, or
whether we just leave ServiceDefUtil.normalize() as the one true entry point
and close this out.
--
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]
