Re: Please have a look at rewriters design

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 17 Mar 2020, at 02:49, thierry bordaz <tbordaz@xxxxxxxxxx> wrote:
> 
> Hi,
> 
> As a follow up of the PR https://pagure.io/389-ds-base/pull-request/50939,
> I wrote down a small design about  rewriters (filter/computed_attr) plugin: http://www.port389.org/docs/389ds/design/search_rewriters.html
> 
> Comments are welcome

Probably the most dangerous thing to say in all of history?

Like, your design is very smart, but that cleverness and flexibility carries many risks. The problem at hand is rewriting ad attributes - not to make a framework. I still say focus on that problem alone rather than trying to solve a generic class of problems.

Anyway, I still don't think this is the right avenue. There are two major reasons for this:

First, is the attempt to make a "generic framework" to solve a "specific problem". We should not have a generic rewrite framework, when all we need is a specific, focused, module just for doing known and well tested attribute transformations. 

Code like COS or MEP may be generic, and it solves many cases but the surface area is huge, it's hard to test, and it's hard to reason about. 

We do not have a need for allowing generic, and arbitrary rewriters to exist, especially not when you have to "compile in" the rewriters anyway!

This should be simply, an "ad rewrite" plugin, where all it does is that one thing - rewrite the attributes as required for AD emulation for IPA. This is far easier to deploy, test and reason about. Ideally, the configuration is simply "the plugin is enabled or disabled".


Second, is the idea of this being a "search rewriter". I don't think this is a good idea. The search path should be simple, it's our hot path. We have many things that have to interact like indexes etc. Look at virtual attribute indexing and such and the work needed for COS to have these used? 

This plugin should be on the write path, transforming when a change occurs. This means the code is much simpler, easier to test, and we need no modifications to our read paths. Things like MEP and replication will "just work" as will indexing and much more.


For me to approve this plugin, I really want to see it being a write-path transformation of values into other values, and it should be focused, targeted, and simple. 

I do want to make one thing clear though - I think it's much better that this plugin exist in 389-ds rather than in freeipa. The 389-ds project has better tooling (like ASAN/LSAN), faster testing capability and a group of subject matter experts for code review. I think that if you were to move this to freeipa, you would not have the same level of testing or review quality as here, so I'd prefer to see you put it here. Sure, I might be difficult on this topic, but I do it because I believe there is a better, more robust manner to approach this problem space than currently you are considering. :) 


Thanks,


> 
> best regards
> thierry
> _______________________________________________
> 389-devel mailing list -- 389-devel@xxxxxxxxxxxxxxxxxxxxxxx
> To unsubscribe send an email to 389-devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives: https://lists.fedoraproject.org/archives/list/389-devel@xxxxxxxxxxxxxxxxxxxxxxx

—
Sincerely,

William Brown

Senior Software Engineer, 389 Directory Server
SUSE Labs
_______________________________________________
389-devel mailing list -- 389-devel@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to 389-devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/389-devel@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Directory Announce]     [Fedora Users]     [Older Fedora Users Mail]     [Fedora Advisory Board]     [Fedora Security]     [Fedora Devel Java]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Mentors]     [Fedora Package Review]     [Fedora Art]     [Fedora Music]     [Fedora Packaging]     [CentOS]     [Fedora SELinux]     [Big List of Linux Books]     [KDE Users]     [Fedora Art]     [Fedora Docs]

  Powered by Linux