You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2002/12/10 06:15:27 UTC
Sanity check: removing src_err
I mentioned this in an unrelated thread and got a +1 from Karl, but
I'd like to give people a chance to -1 before I go ahead and do it.
I'd like to remove the src_err field of svn_error_t. We set it in 16
places and use it in one place (libsvn_fs/trail.c line 141; I'm not
including printing it out in svn_handle_error when SVN_DEBUG is set).
We set it variously to errno (7 places), APR_FROM_OS_ERROR() of a
Windows error (4 places), an APR error (3 places), and a Berkeley DB
error (2 places).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
By the way,
What's your beef with it? I seem to remember a previous post wanting to
put a bullet in it:-)
What grief does it cause?
gat
Glenn A. Thompson wrote:
> Hey,
> How do you plan on detecting DEADLOCK Errors?
> I planned on using it quite a bit for the Abstracted SQL DB stuff.
> In fact I already have code which uses that field to deal with DB
> specifc error codes in abstracted ways.
> If you remove, please suggest something to take it's place.
> If you look through my mega FS patch you will see that I have also
> abstracted it's use for trail.c
> I refer to it in some sort of is_retryable callback implemented as a
> vtable function.
> I'm not sure what I would do without it.
> http://www.cdrguys.com/subversion/index.html
> I am going to start spliting that thing up into smaller pieces this week.
>
>
> gat
>
> Greg Hudson wrote:
>
>> I mentioned this in an unrelated thread and got a +1 from Karl, but
>> I'd like to give people a chance to -1 before I go ahead and do it.
>>
>> I'd like to remove the src_err field of svn_error_t. We set it in 16
>> places and use it in one place (libsvn_fs/trail.c line 141; I'm not
>> including printing it out in svn_handle_error when SVN_DEBUG is set).
>> We set it variously to errno (7 places), APR_FROM_OS_ERROR() of a
>> Windows error (4 places), an APR error (3 places), and a Berkeley DB
>> error (2 places).
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>> For additional commands, e-mail: dev-help@subversion.tigris.org
>>
>>
>>
>>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
Wooooo
Every one is going in different directions and it's my fault:-)
Greg Hudson wrote:
>On Tue, 2002-12-10 at 12:20, Glenn A. Thompson wrote:
>
>
>>I understand his point. But I won't be putting Oracle or MySQL error
>>codes into APR space. I think it would be confusing. Often times, n
>>number of errors can be treated as a single condition. That doesn't
>>mean I want to lose the granularity/information altogether.
>>
>>
>
>In the general case, the fine-grained information in a third-party error
>might be more complicated than a single integer. So if we want an
>svn_error_t to contain this information (and I'm not sure we do),
>src_err isn't a very good way of doing it.
>
>
I do and I agree tha src_err isn't a great way of doing it. Put simply,
it's the way it *was* being done. So I used it in the SQL support
classes I'm developing.
>
>
>>The bottom line is that I'm OK with the change. If I need this type of
>>behavior, and I think I will I'll just create an ERROR code like I
>>mentioned in my later e-mail response.
>>
>>
>
>I don't quite understand this proposal. If you mean that there would be
>an APR error code which says "child->apr_err is a third-party package's
>error code," then I find that hackish.
>
>
I'm saying that if I need to stash some fine-grained error codes I will
simply have an svn error code where I know how to parse the message.
svn_error_create (GENRAL_FS_ERROR,0,
svn_error_create(SVN_DB_INTERNAL_ERROR,0,NULL,"ORA-0001"), "Badness
occurred");
And yes it is hackish. I think I would prefer a separate function to
set (void) user_data on the error. I thinks it's more clear. As I
understand it, marshaling this over the various layers becomes a little
ugly. So, do what you need to do to clean that up. I'll make due.
>I also don't understand the case for error-handling callbacks. If an
>error requires special processing by an inner module, why not perform
>that processing before passing back the error in the first place?
>
>
>
Because sometimes ordering of error handling is important. The
framework needs to do something before the specific impl does his thing.
If the framework is driving the boat, the impl may need to stash some
opaque data in the error structure for a later callback.
But I'm done not making my point:-) We could go all day on theory. But
I've only given myself 2 hours a day for Subversion right now. I'd like
to use them breaking down my patch:-)
You asked for comments on the src_err field. It isn't what I prefer but
it did provide me some value in my undelivered code. I have another way
to do it.
I will do that. I can't vote anyway. I've spent enough of everyone's time.
gat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by Greg Hudson <gh...@MIT.EDU>.
On Tue, 2002-12-10 at 12:20, Glenn A. Thompson wrote:
> I understand his point. But I won't be putting Oracle or MySQL error
> codes into APR space. I think it would be confusing. Often times, n
> number of errors can be treated as a single condition. That doesn't
> mean I want to lose the granularity/information altogether.
In the general case, the fine-grained information in a third-party error
might be more complicated than a single integer. So if we want an
svn_error_t to contain this information (and I'm not sure we do),
src_err isn't a very good way of doing it.
> The bottom line is that I'm OK with the change. If I need this type of
> behavior, and I think I will I'll just create an ERROR code like I
> mentioned in my later e-mail response.
I don't quite understand this proposal. If you mean that there would be
an APR error code which says "child->apr_err is a third-party package's
error code," then I find that hackish.
I also don't understand the case for error-handling callbacks. If an
error requires special processing by an inner module, why not perform
that processing before passing back the error in the first place?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
Hey,
Karl Fogel wrote:
>"Glenn A. Thompson" <gt...@cdr.net> writes:
>
>
>>I guess I could map them onto DEADLOCK or create other ERRORs. I'm
>>not to thrilled about mapping every error condition into APR
>>space. How about an assessor to set the field?
>>
>>
>
>I think Greg's point is that we don't have to map every error, we only
>have to map the ones our code checks for.
>
>And for those, it makes sense to create dedicated SVN_ERR_FOO codes
>anyway -- after all, that was the whole point of that error space, and
>also it's consistent with the way we've been documenting error returns
>so far (e.g., "This returns SVN_ERR_FOO if the file system deadlocks").
>
>
I understand his point. But I won't be putting Oracle or MySQL error
codes into APR space. I think it would be confusing. Often times, n
number of errors can be treated as a single condition. That doesn't
mean I want to lose the granularity/information altogether. So I will
check for them in the code and create a svn error. Now, consider a
situation where I have an abstracted framework. Lets say that I trap a
DB error and classify it as x ,y or z in APR error space. I return to
the frame work. He in turn may need to call back to a impl to "handle
the error" after he does some framework stuff. What if the impl error
handler needs more detail?
The bottom line is that I'm OK with the change. If I need this type of
behavior, and I think I will I'll just create an ERROR code like I
mentioned in my later e-mail response. I'll decode it's content if
needed. And it most likely will be needed. With regard to the
DEADLOCK. DEALOCKs are not the only reason a process will be rolled
back in the SQL world. That is why my trail patch changed the
terminology. Another interesting thing is under SQL DBs you don't
always control the "abort" you are rolled back and then told about it.
You can issue a rollback, but you are not always given the choice.
There are also isolation levels and other things which affect when you
get rolled back. At commit, or when the statement is executed.
However, I am confident that I can map SQL onto the trial concept for
my first SQL impl. I'm still not sure whether or not I will use impls
for the "non baseline based" sql impl.
gat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Glenn A. Thompson" <gt...@cdr.net> writes:
> I guess I could map them onto DEADLOCK or create other ERRORs. I'm
> not to thrilled about mapping every error condition into APR
> space. How about an assessor to set the field?
I think Greg's point is that we don't have to map every error, we only
have to map the ones our code checks for.
And for those, it makes sense to create dedicated SVN_ERR_FOO codes
anyway -- after all, that was the whole point of that error space, and
also it's consistent with the way we've been documenting error returns
so far (e.g., "This returns SVN_ERR_FOO if the filesystem deadlocks").
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
>
> It seems to make sense to implement some sort of logging routine.
errrrr
s/to implement/for implementing/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
Greg Hudson wrote:
>On Tue, 2002-12-10 at 01:32, Glenn A. Thompson wrote:
>
>
>>Hey,
>>How do you plan on detecting DEADLOCK Errors?
>>
>>
>
>Probably the easiest way is a dedicated svn_fs error code for deadlocks.
>
>As to why I dislike src_err: it is an extra argument to every
>svn_error_create call for the sake of a vanishingly small number of
>cases, its value lives in an indeterminate number space, and it renders
>the use of the error system prone to confusing inconsistencies
>
There could conceivably be other errors other than DEADLOCK that would
be retryable.
I guess I could map them onto DEADLOCK or create other ERRORs. I'm not
to thrilled about mapping every error condition into APR space. How
about an assessor to set the field? Passing a DB error code through for
printing, does have some usefulness. I mean Oracle has tons of them.
They even have a program to given a verbal explanation of the error. I
would actually prefer a char field. DBAs may want these codes. They
couldn't care less about it being mapped onto APR_ERRORs. They may
need to resize a sort area or something similar. Errors can be
meaningful to the right person even if it is in indeterminate number space.
While we are talking about Error stuff:
svn_fs_set_berkeley_errcall never gets called.
I hate it as much as you hate src_err:-)
It seems to make sense to implement some sort of logging routine. Why not do just that.
Call it with a file handle to log to. This could be abstracted for other FS implementations as well.
gat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
Hmmm,
A thought; how about having a SVN_EXTERNAL error code. So, a program
could traverse the errors looking for SVN_EXTERNA if it needs to get at
it later. This would allow me to put anything I want in there. Chars too!
When you want to keep a src err simply create a SVN_EXTERNAL wrapped by
whatever the SVN_MAPPING would be?
So I could create a error like "ORA-0001". Or "MySql-0001". I guess I
can do this anyway. But it seems nice to identify certain errors as
"from external sources".
gat
Greg Hudson wrote:
>On Tue, 2002-12-10 at 01:32, Glenn A. Thompson wrote:
>
>
>>Hey,
>>How do you plan on detecting DEADLOCK Errors?
>>
>>
>
>Probably the easiest way is a dedicated svn_fs error code for deadlocks.
>
>As to why I dislike src_err: it is an extra argument to every
>svn_error_create call for the sake of a vanishingly small number of
>cases, its value lives in an indeterminate number space, and it renders
>the use of the error system prone to confusing inconsistencies.
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by Greg Hudson <gh...@MIT.EDU>.
On Tue, 2002-12-10 at 01:32, Glenn A. Thompson wrote:
> Hey,
> How do you plan on detecting DEADLOCK Errors?
Probably the easiest way is a dedicated svn_fs error code for deadlocks.
As to why I dislike src_err: it is an extra argument to every
svn_error_create call for the sake of a vanishingly small number of
cases, its value lives in an indeterminate number space, and it renders
the use of the error system prone to confusing inconsistencies.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sanity check: removing src_err
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
Hey,
How do you plan on detecting DEADLOCK Errors?
I planned on using it quite a bit for the Abstracted SQL DB stuff.
In fact I already have code which uses that field to deal with DB
specifc error codes in abstracted ways.
If you remove, please suggest something to take it's place.
If you look through my mega FS patch you will see that I have also
abstracted it's use for trail.c
I refer to it in some sort of is_retryable callback implemented as a
vtable function.
I'm not sure what I would do without it.
http://www.cdrguys.com/subversion/index.html
I am going to start spliting that thing up into smaller pieces this week.
gat
Greg Hudson wrote:
>I mentioned this in an unrelated thread and got a +1 from Karl, but
>I'd like to give people a chance to -1 before I go ahead and do it.
>
>I'd like to remove the src_err field of svn_error_t. We set it in 16
>places and use it in one place (libsvn_fs/trail.c line 141; I'm not
>including printing it out in svn_handle_error when SVN_DEBUG is set).
>We set it variously to errno (7 places), APR_FROM_OS_ERROR() of a
>Windows error (4 places), an APR error (3 places), and a Berkeley DB
>error (2 places).
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org