Introduce more specific exceptions, like NotFound, AlreadyExists, BadRequest, PermissionDenied, InternalError, and others

[nfx]

Improve the ergonomics of SDK, where instead of except DatabricksError as err: if err.error_code != 'NOT_FOUND': raise err else: do_stuff() we could do except NotFound: do_stuff(), like in this example.

Additionally, it'll make it easier to read stack traces, as they will contain specific exception class name. Examples of unclear stack traces are: databrickslabs/ucx#359, databrickslabs/ucx#353, databrickslabs/ucx#347,

First principles

• do not override builtins.NotImplemented for NOT_IMPLEMENTED error code
• assume that platform error_code/HTTP status code mapping is not perfect and in the state of transition
• we do represent reasonable subset of error codes as specific exceptions
• it's still possible to access error_code from specific exceptions like NotFound or AlreadyExists.

Proposal

• have hierarchical exceptions, also inheriting from Python's built-in exceptions
• more specific error codes override more generic HTTP status codes
• more generic HTTP status codes matched after more specific error codes, where there's a default exception class per HTTP status code, and we do rely on Databricks platform exception mapper to do the right thing.
• have backward-compatible error creation for cases like using older versions of the SDK on the way never releases of the platform.

Naming conflict resolution

We have four sources of naming and this is a proposed order of naming conflict resolution:

1. Databricks error_code, that is surfaced in our API documentation, known by Databricks users
2. HTTP Status codes, known by some developers
3. Python builtin exceptions, known by some developers
4. grpc error codes https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto#L38-L185, know by some developers

[renardeinside]

Agree with this statement:

HTTP Status Codes vs Error Codes (preferred)

IMO, error codes shall be the main body of the error, since they provide logical explanation to what has happened, while HTTP status codes provide in-depth technical details.

Comparing two options:

except MetastoreDoesNotExist:
    # do something

And:

except NotFound:
  # do something

The first one sounds a bit more DDD which is proven to be a good practice.

[nfx]

@renardeinside technically, we can do class MetastoreDoesNotExist(NotFound): pass

[nfx]

some more input: SCIM doesn't have a concept of AlreadyExists, only ResourceConflict.

12:26 DEBUG [d.s] POST /api/2.0/preview/scim/v2/Groups
> {
>   "displayName": "db-temp-ucx_fY5N",
>   "members": [
>     {
>       "display": "test-user-2",
>       "value": "**REDACTED**"
>     },
>     {
>       "display": "test-user-3",
>       "value": "**REDACTED**"
>     },
>     "... (1 additional elements)"
>   ],
>   "meta": {
>     "resourceType": "WorkspaceGroup"
>   }
> }
< 409 Conflict
< {
<   "detail": "Group with name db-temp-ucx_fY5N already exists.",
<   "schemas": [
<     "urn:ietf:params:scim:api:messages:2.0:Error"
<   ],
<   "status": "409"
< }

[nfx]

and first time seeing this:

E               databricks.sdk.core.DatabricksError: The service at /api/2.0/pipelines is temporarily unavailable. Please try again later. [TraceId: 00-b31ec4becd0d5db8df40d3aa6c7b8d66-f9f0e60e28e5ac19-00]

[alexott]

Yes, it makes sense to have very specific errors and handle them explicitly

[william-conti]

We're in this situation in the UCX project and it's starting to be urgent because of the usage customer has of this tool. Please prioritize this when possible

[nfx]

Now this PR is generated from the metadata in databricks/databricks-sdk-go#682

[mgyucht]

Nice, thanks! Couple small suggestions, otherwise I think we're good.

[mgyucht]

Is this necessary? These are also the only exported members of this module. I also wasn't aware that this variable was respected in modules that also define an __init__.py

[nfx]

this is how python works ;)

[mgyucht]

I just cloned this PR and removed this line and was still able to import these:

from databricks.sdk import errors

print(dir(errors))

['Aborted', 'AlreadyExists', 'BadRequest', 'Cancelled', 'DataLoss', 'DatabricksError', 'DeadlineExceeded', 'ErrorDetail', 'InternalError', 'InvalidParameterValue', 'NotFound', 'NotImplemented', 'OperationFailed', 'OperationTimeout', 'PermissionDenied', 'RequestLimitExceeded', 'ResourceAlreadyExists', 'ResourceConflict', 'ResourceExhausted', 'TemporarilyUnavailable', 'TooManyRequests', 'Unauthenticated', 'Unknown', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__', 'base', 'error_mapper', 'mapper', 'mapping', 'sdk']

If you want to control what is exported, I think you need to include this in the __init__.py file explicitly.

[mgyucht]

This LGTM. Thanks for working with us on this!