Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DPP-701] Maybe missed error code conversions #11510

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ object ScenarioServiceMain extends App {

object ScenarioService {
private def notFoundContextError(id: Long): StatusRuntimeException =
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language team

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Here and other places helpfully annotated with "Language team" by Tudor)

@remyhaemmerle-da We spotted these places that could need adapting to self service error codes (aka V2). Are you aware of these in the Language team (if there're legitimate)?

Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scenarios will die completely in Daml 2.0, safe to ignore.

Status.NOT_FOUND.withDescription(s" context $id not found!").asRuntimeException
}

Expand Down Expand Up @@ -292,8 +293,10 @@ class ScenarioService(implicit

} catch {
case e: archive.Error =>
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language team

Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

respObs.onError(Status.INVALID_ARGUMENT.withDescription(e.msg).asRuntimeException())
case NonFatal(e) =>
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language team

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

respObs.onError(Status.INTERNAL.withDescription(e.toString).asRuntimeException())
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ class JsonLedgerClient(
// TODO (MK) Using a grpc exception here doesn’t make that much sense.
// We should refactor this to provide something more general.
Future.successful(
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language or runtime team?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanobaghino-da @remyhaemmerle-da We spotted this place that could need adapting to self service error codes (aka V2). Are you aware of this in the Language or Runtime team (if it's legitimate)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language or runtime team?

Language is responsible from this part of the script. @stefanobaghino-da you can ignore this one.

Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can ignore this one.
cc @cocreature

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that’s fine, just keep it

Left(new StatusRuntimeException(Status.UNKNOWN.withDescription(errors.toString)))
)
case ErrorResponse(errors, status) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ final class TransactionsServiceImpl(ledgerContent: Observable[LedgerItem])
Metadata.Key.of("cause", Metadata.ASCII_STRING_MARSHALLER),
s"BEGIN should be strictly smaller than END. Found BEGIN '${request.getBegin}' and END '${request.getEnd}'",
)
// TODO error codes: Adapt V2 ?
stefanobaghino-da marked this conversation as resolved.
Show resolved Hide resolved
responseObserver.onError(Status.INVALID_ARGUMENT.asRuntimeException(metadata))
} else {
ledgerContent.subscribe(new Observer[LedgerItem] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ class RejectionGenerators(conformanceMode: Boolean) {
// TODO error codes: This converter is deprecated and should be removed
// Instead of using this, construct proper validation errors in callers of this method
// and only convert to StatusRuntimeExceptions when dispatched (e.g. in ApiSubmissionService)
@deprecated("not used in daml sdk")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

def validationFailure(reject: StatusRuntimeException)(implicit
contextualizedErrorLogger: ContextualizedErrorLogger
): StatusRuntimeException = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ object CompletionResponse {
}

private[daml] def toException(response: TrackedCompletionFailure): StatusException =
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like all command-tracker stuff, it is incorporated both into the ledger-api-server and the client code. As such we need to keep the runtime team in the loop.
@stefanobaghino-da FYI

response match {
case QueueCompletionFailure(failure) =>
val metadata = extractMetadata(failure)
Expand Down Expand Up @@ -115,6 +116,7 @@ object CompletionResponse {

private def buildException(metadata: Map[String, String], status: StatusJavaProto.Builder) = {
val details = mergeDetails(metadata, status)
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team

protobuf.StatusProto.toStatusException(
status
.clearDetails()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
ErrorDetails.RequestInfoDetail("trace-id")

"ErrorFactories" should {
"do it" in {
Code.values().foreach(println)
fail()
}
"return sqlTransientException" in {
val failureReason = "some db transient failure"
val someSqlTransientException = new SQLTransientException(failureReason)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ private[services] final class QueueBackedTracker(
_.left.map(completionFailure => QueueCompletionFailure(completionFailure))
)
case QueueOfferResult.Failure(t) =>
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me

failedQueueSubmission(
GrpcStatus.ABORTED
.withDescription(s"Failed to enqueue: ${t.getClass.getSimpleName}: ${t.getMessage}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ private[platform] object Conversions {
import RejectionReasonOps._

def toParticipantStateRejectionReason: state.Update.CommandRejected.RejectionReasonTemplate =
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team: used in sandbox-classic

rejectionReason match {
case domain.RejectionReason.Inconsistent(reason) =>
newRejectionReason(Status.Code.ABORTED, s"Inconsistent: $reason")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,15 @@ case class DefaultBatchingQueue(
)
)
case f: QueueOfferResult.Failure =>
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KV committer team

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Here and other places helpfully annotated with "KV committer team" by Tudor)

@fabiotudone-da We spotted these places that could need adapting to self service error codes (aka V2). Are you aware of these in the Committer team (if there're legitimate)?

SubmissionResult.SynchronousError(
Status(
Code.INTERNAL.value,
f.toString,
)
)
case QueueOfferResult.QueueClosed =>
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KV committer team

SubmissionResult.SynchronousError(
Status(
Code.INTERNAL.value,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,15 @@ class ValidatingCommitter[LogResult](
postCommit(value)
SubmissionResult.Acknowledged
case Left(MissingInputState(keys)) =>
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KV committer team

SubmissionResult.SynchronousError(
Status(
Code.INTERNAL.value,
s"Missing input state: ${keys.map(_.bytes.asScala.map("%02x".format(_)).mkString).mkString(", ")}",
)
)
case Left(ValidationError(reason)) =>
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KV committer team

SubmissionResult.SynchronousError(
Status(
Code.INTERNAL.value,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class BatchedValidatingCommitter[LogResult](
Future.successful(SubmissionResult.Acknowledged)
case Failure(exception) =>
Future.successful(
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KV Committer team

SubmissionResult
.SynchronousError(Status(Code.INTERNAL.value, exception.getLocalizedMessage))
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ object SubmissionResult {
override val description: String = s"Submission failed with error ${status.message}"

def exception: StatusRuntimeException =
// TODO error codes: Adapt V2 ?
Copy link
Contributor

@tudor-da tudor-da Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are already getting a Status, expecting that the WriteService is returning self-service error code formatted statuses. I'll look into it, thanks 👍

protobuf.StatusProto.toStatusRuntimeException(GrpcStatus.toJavaProto(status))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ object Rejection {
domain.RejectionReason.InvalidLedgerTime(description)

override lazy val toStateRejectionReason: state.Update.CommandRejected.RejectionReasonTemplate =
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team

state.Update.CommandRejected.FinalReason(
RpcStatus.of(
code = Status.Code.ABORTED.value(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,7 @@ private[sandbox] final class InMemoryLedger(
loggingContext: LoggingContext
): Future[Unit] =
// sandbox-classic in-memory ledger does not support pruning
// TODO error codes: Adapt V2 ?
tudor-da marked this conversation as resolved.
Show resolved Hide resolved
Future.failed(Status.UNIMPLEMENTED.asRuntimeException())
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class SandboxResetService(
serverCallHandler: ServerCallHandler[ReqT, RespT],
): Listener[ReqT] = {
if (resetInitialized.get) {
// TODO error codes: Adapt V2 ?
Copy link
Contributor

@tudor-da tudor-da Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team

throw new StatusRuntimeException(
Status.UNAVAILABLE.withDescription("Sandbox server is currently being reset")
)
Expand Down Expand Up @@ -71,6 +72,7 @@ class SandboxResetService(
logger.info("Initiating server reset.")

if (!resetInitialized.compareAndSet(false, true))
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team

throw new StatusRuntimeException(
Status.FAILED_PRECONDITION.withDescription("Sandbox server is currently being reset")
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ object ReadWriteServiceBridge {
)
case QueueOfferResult.Failure(throwable) =>
logger.error("Error enqueueing new submission.", throwable)
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team

SubmissionResult.SynchronousError(
Status(
Code.INTERNAL.value,
Expand All @@ -269,6 +270,7 @@ object ReadWriteServiceBridge {
)
case QueueOfferResult.QueueClosed =>
logger.error("Error enqueueing new submission: queue is closed.")
// TODO error codes: Adapt V2 ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Participant team

SubmissionResult.SynchronousError(
Status(
Code.INTERNAL.value,
Expand Down