Skip to content

Commit 0eb3c55

Browse files
committed
[CHORE] Say what the code does rather than what it stopped doing
Three comments described the change relative to the previous shape: a bound that is "unchanged", a lock that was taken out, and an ordering that "no longer" comes from the curl client. A reader of the file has no previous shape to compare against, and the rationale for the change is in this PR. Each now states the current fact: what response_timeout_ bounds, that nothing serialises ForceFlush against Shutdown, and that the curl client does not produce the ordering the case pins while another client may. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1 parent f6fa2bb commit 0eb3c55

2 files changed

Lines changed: 11 additions & 13 deletions

File tree

‎exporters/elasticsearch/src/es_log_record_exporter.cc‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -606,12 +606,10 @@ bool ElasticsearchLogRecordExporter::ForceFlush(std::chrono::microseconds timeou
606606
timeout = opentelemetry::common::DurationUtil::AdjustWaitForTimeout(
607607
timeout, std::chrono::microseconds::zero());
608608

609-
// A caller that asks for no deadline still gets one, because a flush that cannot end is worse
610-
// than one that reports it did not finish, and the client is not obliged to call back at all.
611-
// response_timeout_ is the bound this wait already had before the accounting was fixed, and it
612-
// is the same value the request itself is given, so a session outstanding when the watermark is
613-
// taken has at most that long before the client owes it a terminal event. The bound is
614-
// unchanged; what changes is that running out of it no longer reports success.
609+
// A caller that asks for no deadline still gets one: a flush that cannot end is worse than one
610+
// that reports it did not finish, and a client is not obliged to call back at all.
611+
// response_timeout_ is the same value the request itself is given, so a session outstanding when
612+
// the watermark is taken has at most that long before the client owes it a terminal event.
615613
if (timeout <= std::chrono::microseconds::zero())
616614
{
617615
timeout = std::chrono::seconds{options_.response_timeout_};

‎exporters/elasticsearch/test/es_log_record_exporter_test.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -761,10 +761,10 @@ TEST_F(ElasticsearchForceFlushTests, ANoDeadlineFlushGivesUpAtTheResponseTimeout
761761
EXPECT_GE(ms, 1000) << "returned without waiting, so the bound is not the response timeout";
762762
}
763763

764-
// ForceFlush() and Shutdown() are required to be safe to call concurrently, and this PR took out
765-
// the lock that used to serialise them, so the overlap is deterministic here rather than left to
766-
// chance: the flush is parked on its session before the shutdown starts. A real client answers
767-
// its outstanding sessions while shutting down, which is what has to wake the waiter.
764+
// ForceFlush() and Shutdown() have to be safe to call concurrently and nothing serialises them,
765+
// so the overlap here is arranged rather than left to chance: the flush is parked on its session
766+
// before the shutdown starts. A real client answers its outstanding sessions while shutting down,
767+
// which is what has to wake the waiter.
768768
TEST_F(ElasticsearchForceFlushTests, AParkedFlushIsWokenByTheShutdownThatCompletesItsSession)
769769
{
770770
std::shared_ptr<http_client::EventHandler> captured;
@@ -1315,9 +1315,9 @@ TEST_F(ElasticsearchAsyncCompletionTests, TwoTerminalEventsCountAsOneSession)
13151315
<< "the second session is still in flight";
13161316
}
13171317

1318-
// The curl client's completion lambda tests the response first and the abort in an else, so this
1319-
// ordering no longer comes from it. It still comes from anywhere: EventHandler promises callers
1320-
// nothing about how many terminal events arrive, and #4360 is open on exactly that.
1318+
// The curl client's completion lambda tests the response first and the abort in an else, so it
1319+
// does not produce this ordering. Another client may: EventHandler promises callers nothing about
1320+
// how many terminal events arrive, and #4360 is open on exactly that.
13211321
TEST_F(ElasticsearchAsyncCompletionTests, CancelledThenAResponseCountsOnce)
13221322
{
13231323
std::vector<std::shared_ptr<http_client::EventHandler>> kept;

0 commit comments

Comments
 (0)