directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Emmanuel Lécharny <elecha...@gmail.com>
Subject Re: Flexible schema error handling
Date Wed, 11 Jul 2018 23:25:54 GMT


Le 11/07/2018 à 17:40, Radovan Semancik a écrit :
> Hi,
> 
> I have pushed my changes. It is pretty simple from the user point of view:
> 
> defaultSchemaManager.setErrorHandler(schemaErrorHandler);
> 
> I have removed old List<Throwable> errors and replaced it with
> SchemaErrorHandler interface. I have figured that it is actually OK to
> pay a cost of indirection here. It will only impact the servers with bad
> schema, and those servers deserve to suffer anyway, right? :-) The real
> reason was that by always using SchemaErrorHandler (and providing
> default logging implemenation) I was able to somehow simplify and unify
> error handling.

Good.

> 
> However, I have encountered two issues:
> 
> 1) There are two dissociateFromSchema() methods in Registries. One of
> them is used, the other seems not to be used at all. The difference was
> in the parameters. When I removed the "errors" parameter then those
> method clashed. They seem to do quite different things, so they probably
> should not have the same name anyway (if both are really used). So I
> have renamed the wrong one to dissociateFromSchemaQuestionable().
> Emmanual, this is probably something you should have a look at. I feel a
> bit lost here.

Whoaa... You are doing some archeology here :-)


It's almost a 10 years old code (OMG, I can't believe I wrote that
already 9 years ago...), and my guess is that when I added the list of
throwable, I forgot to remove the other method, which hasn't eveolved
since then, which explain why the code is so different.

I thi,k we are safe if you simply remove the method you have renamed...
> 
> 2) Ava. I think Ava is supposed to be a simple object, almost like a
> DTO. 

Not sure what you mean by DTO in this context... Because it's serializable ?


It is a bit tangled with SchemaManager ... but I haven't found any
> good way how to implement flexible logging in Ava (I do not want to
> pollute SchemaManager interface with SchemaErrorHandler). 

Ava are used in many parts of the server, this makes it a bit complex.
Typically, we need to have a schema awat-re Ava in the server, while the
client might not have any schema.

I'm not sure we need flexible logging in Ava though: it's enough to
throw an exception with the error message, as we do.

But I have
> realized that Ava is such a simple object, it probably should not log
> the errors at all.

I don't disagree... It was helpfull when I wrote the class, because
there are so many corner cases to deal wth, and the logs helped me back
then. Now, it's a bit spurious.

 It is throwing exceptions anyway. So I think it would
> be responsibility of the caller to log errors (exceptions). So I have
> removed the logging statements from Ava constructors. I hope that this
> is OK. 

I do think it's OK.



I have also found potential issues with Ava
> comparison/normalization .... I have put a comment in the source code.
> This is probably something to have a look in the future.

Actually, it's the equals() method, and in the comparison done when we
have a SchemaManager, we have to assume the value of both Ava has
already been normalized. This try-catch is a bit superfluous, we most
certainly can get rid of it...


> 
> Overall, the solution is not perfect. But I'm quite happy with that for
> 2.0. It is definitely better than the temporary hack that we had in 1.0.

Good job. Have you tried to build the server with those changes ? If
not, I will do that tomorrow evening.

Thanks Radovan !


-- 
Emmanuel Lecharny

Symas.com
directory.apache.org


Mime
View raw message