The completion stream RPC defaults to the ledger end as offset #1961
Conversation
stefanobaghino-da
force-pushed
the
ledger-api-completion-stream-offset-optional
branch
from
ce6a963 to
cd646d6
Compare
.../scala/com/digitalasset/platform/server/api/services/grpc/GrpcCommandCompletionService.scala
Outdated
Show resolved
Hide resolved
| private[this] val completionStreamDefaultOffset = | ||
| LedgerOffset.of(LedgerOffset.Value.Boundary(LedgerOffset.LedgerBoundary.LEDGER_END)) | ||
|
|
||
| private def fillInWithDefaults(request: CompletionStreamRequest): CompletionStreamRequest = { |
There was a problem hiding this comment.
nitpicking: this method could live on the class itself, does not have to be "static". Also, you can drop two {a and '}'s
There was a problem hiding this comment.
I usually favor "static" methods unless I'm relying on a specific object's state. I can make it an instance method if you think it belongs there. Do you have a specific reason why you'd prefer it to be an instance method?
There was a problem hiding this comment.
Just so it's close to where it's used. I tend not put too many functions on objects, even if they are stateless, unless they need to have wider visibility. Putting them on the class instance has no runtime penalty and results in better code cohesion in my opinion.
This is a matter of taste I think, don't change it if you don't agree.
...digitalasset/platform/tests/integration/ledger/api/commands/CommandCompletionServiceIT.scala
Show resolved
Hide resolved
...digitalasset/platform/tests/integration/ledger/api/commands/CommandCompletionServiceIT.scala
Outdated
Show resolved
Hide resolved
...digitalasset/platform/tests/integration/ledger/api/commands/CommandCompletionServiceIT.scala
Outdated
Show resolved
Hide resolved
...digitalasset/platform/tests/integration/ledger/api/commands/CommandCompletionServiceIT.scala
Outdated
Show resolved
Hide resolved
- ignore command creation results (#1961 (comment)) - avoid re-connecting to the client for every command (#1961 (comment)) - move offset field optionality to domain object (#1961 (comment))
|
@gerolf-da Your review should be addressed by The same commit also covers two of @gaboraranyossy-da's comments but a couple are still open. One in particular may benefit from some extra attention. |
Fixes #1913 Relevant changes are propagated to the Java bindings (including deprecating a method that would now return a nullable ledger end).
- ignore command creation results (#1961 (comment)) - avoid re-connecting to the client for every command (#1961 (comment)) - move offset field optionality to domain object (#1961 (comment))
stefanobaghino-da
force-pushed
the
ledger-api-completion-stream-offset-optional
branch
from
1f8fae9 to
a331839
Compare
|
I don't know why but this keeps on failing while running the formatting check. Locally everything seems to be in order. Rebasing on the latest |
|
@gaboraranyossy-da Thanks, the tests have been fixed by e8e4b5c |
Fixes #1913
Relevant changes are propagated to the Java bindings (including
deprecating a method that would now return a nullable ledger end).
Pull Request Checklist
NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.