github-actions[bot] commented on code in PR #4547:
URL: https://github.com/apache/iggy/pull/4547#discussion_r4276036264
##########
foreign/csharp/Iggy_SDK/Vsr/VsrConnection.cs:
##########
@@ -341,6 +350,41 @@ private static bool
IsReplayableTransient(IggyInvalidStatusCodeException error,
_ => false
};
}
+
+ /// <summary>
+ /// Whether a refusal of a replay settles the outcome of the original
request. The primary answers a
+ /// same-id replay from its session table, so a result-section
rejection is the cached reply of the
+ /// original, the commit of a request that never entered the pipeline,
or a refusal of a request neither
+ /// the session table nor the pipeline holds. The codes below settle
nothing: most come from the checks
+ /// that run before that lookup, and REQUEST_TOO_OLD means the
original applied and its reply is gone.
+ /// PERSONAL_ACCESS_TOKEN_ALREADY_EXISTS settles even as the refusal
to replay a token the original
+ /// minted: either way the name is taken and the caller holds no
token. Apply can commit INVALID_COMMAND
+ /// too, but nothing tells it apart from the preflight one. The server
never puts HISTORY_UNAVAILABLE in a
+ /// result section, and it stays listed because settling on it would
be wrong if it ever did. The caller
+ /// filters TRANSIENT_NOT_COMMITTED out before asking.
+ /// </summary>
+ private static bool SettlesOutcome(IggyInvalidStatusCodeException error)
+ {
+ return error is
+ {
+ FromResultSection: true,
+ StatusCode: not (VsrError.TRANSIENT_NOT_ACCEPTED or
VsrError.NOT_PRIMARY or VsrError.REQUEST_TOO_OLD
+ or VsrError.INVALID_COMMAND or VsrError.UNAUTHENTICATED or
VsrError.HISTORY_UNAVAILABLE)
+ };
+ }
+
+ /// <summary>
+ /// Whether an unsettled refusal of a replay is final: the primary's
preflight answers every replay of the
+ /// same request id with it again, so replaying until the deadline
only ends in the same unknown outcome.
+ /// </summary>
+ private static bool IsFinalRefusal(IggyInvalidStatusCodeException error)
+ {
+ return error is
+ {
+ FromResultSection: true,
+ StatusCode: VsrError.REQUEST_TOO_OLD or VsrError.INVALID_COMMAND
Review Comment:
nit: The partition plane answers an aged-out replay with `RequestTooOld` on
the status word. `IsFinalRefusal` requires `FromResultSection`, so that shape
replays every 50 ms until the read deadline instead of ending at once. Fix:
match the status-word shape too, because the same request id draws the same
terminal refusal every time.
##########
foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.PollRouting.cs:
##########
@@ -249,8 +256,25 @@ private async Task<IMemoryOwner<byte>> SendRoutedAsync(int
code, SendTarget targ
catch (OperationCanceledException error) when
(!token.IsCancellationRequested)
{
RemovePartitionRoute(target.Key);
+ // The budget can expire in the backoff or in a read-only control
request that follows it, before the
+ // send call for the request starts again, so the last refusal
still decides the outcome. An expiry
+ // inside that send call reports an unknown outcome even before
the write, which is conservative.
+ if (refusal is not null)
+ {
+ ExceptionDispatchInfo.Throw(refusal);
+ }
+
throw new VsrRequestOutcomeUnknownException(error);
}
+ // The read timer of a route lookup fires at the same deadline as the
budget and turns into a timeout
+ // IOException when it wins. A kept refusal means no send started
since, so it still decides the outcome.
+ catch (IOException) when (refusal is not null
Review Comment:
nit: The new filter catches only `IOException`, so a `SocketException` from
the redial inside `SendPollControlAsync` still escapes the method instead of
the kept refusal. Fix: widen the filter to the connection failures, for example
`SocketException`, because a lookup that fails before any send cannot change
the request outcome.
##########
foreign/csharp/Iggy_SDK/Vsr/VsrError.cs:
##########
@@ -59,11 +60,17 @@ internal static IggyInvalidStatusCodeException
Exception(int code, string messag
}
/// <summary>
- /// A verdict the server reported, on the reply status word, in the
committed result section or in an
- /// eviction frame. Only these may drive retry and failover.
+ /// A verdict the server reported, on the reply status word or in an
eviction frame. Only server verdicts,
Review Comment:
nit: This comment claims that only server verdicts drive retry and failover.
`PollNotAccepted()` raises `TRANSIENT_NOT_ACCEPTED` with `FromServer` false,
and the routed retry acts on it. Fix: name that exception as the client-raised
case, or narrow the claim.
##########
foreign/csharp/Iggy_SDK_Tests/VsrTests/PrimaryPollingTests.cs:
##########
@@ -228,6 +228,37 @@ public async Task
given_only_refusals_when_the_poll_budget_expires_should_report
Assert.All(cluster.Primaries, primary => Assert.Equal(0,
primary.Connections));
}
+ [Fact]
+ public async Task
given_primary_refusal_when_the_budget_expires_in_the_next_route_lookup_should_report_not_accepted()
+ {
+ using var cluster = new PollCluster();
+ var release = new
TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+ var lookups = 0;
+ cluster.OnPrimaryPoll = request => Reply(request.Operation, [],
TRANSIENT_NOT_ACCEPTED);
+ cluster.OnCoordinator = request =>
+ {
+ if (request.Code == CommandCodes.GET_PARTITION_ROUTE_CODE &&
Interlocked.Increment(ref lookups) == 2)
+ {
+ release.Task.GetAwaiter().GetResult();
Review Comment:
nit: This test blocks its second route lookup until `VsrRequestTimeoutMs`
(30 s) expires, so one unit test costs half a minute and holds a mock handler
for that time. Fix: make the request budget injectable and set a short value in
this test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
