Skip to content
31 changes: 29 additions & 2 deletions velox/connectors/kafka/KafkaDataSource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ namespace facebook::velox::connector::kafka {
namespace {
constexpr const char* kTaskIndex = "task_index";
constexpr const char* kTaskParallelism = "parallelism";
constexpr uint32_t kPollTimeoutBackoffMillis = 1;

uint32_t javaStringHashCode(const std::string& value) {
uint32_t hash = 0;
Expand Down Expand Up @@ -58,6 +59,7 @@ KafkaDataSource::KafkaDataSource(
config_(config),
outputType_(outputType),
batchSize_(config_->getDataBatchSize()) {
scheduler_.start();
VELOX_CHECK(batchSize_ > 0, "Batch size config value must greater than 0.");
const std::shared_ptr<KafkaTableHandle> kafkaTableHandle =
std::dynamic_pointer_cast<KafkaTableHandle>(tableHandle);
Expand All @@ -80,6 +82,11 @@ KafkaDataSource::KafkaDataSource(
createRecordDeserializer(config_->getFormat(), outputType_);
}

KafkaDataSource::~KafkaDataSource() {
scheduler_.shutdown();
completeBlockingFuture();
}

bool KafkaDataSource::consumerCanbeCreated() {
return config_->exists(ConnectionConfig::kBootstrapServers) &&
config_->exists(ConnectionConfig::kClientId) &&
Expand Down Expand Up @@ -230,9 +237,29 @@ void KafkaDataSource::applyTaskScopedClientId() {
fmt::format("{}-{}", config_->getClientId(), taskIndex)}});
}

void KafkaDataSource::completeBlockingFuture() {
if (blockingPromise_.has_value()) {
blockingPromise_->setValue();
blockingPromise_.reset();
}
}

std::optional<RowVectorPtr> KafkaDataSource::blockOnPollTimeout(
velox::ContinueFuture& future) {
auto [promise, blockedFuture] =
makeVeloxContinuePromiseContract("KafkaDataSource::next");
blockingPromise_ = std::move(promise);
scheduler_.addFunctionOnce(
[this]() { completeBlockingFuture(); },
fmt::format("KafkaDataSource::next.{}", ++blockingSequence_),
std::chrono::milliseconds(kPollTimeoutBackoffMillis));
future = std::move(blockedFuture);
return std::nullopt;
}

std::optional<RowVectorPtr> KafkaDataSource::next(
uint64_t,
velox::ContinueFuture&) {
velox::ContinueFuture& future) {
std::optional<RowVectorPtr> res;
size_t consumedMsgBytes = 0;
if (queue_.empty()) {
Expand All @@ -242,7 +269,7 @@ std::optional<RowVectorPtr> KafkaDataSource::next(
consumePos_ = 0;
// If nothing consumed, return directly.
if (consumedMsgBytes == 0) {
return res;
return blockOnPollTimeout(future);
}
}
outRow_->prepareForReuse();
Expand Down
11 changes: 11 additions & 0 deletions velox/connectors/kafka/KafkaDataSource.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
#pragma once

#include <cppkafka/cppkafka.h>
#include <folly/experimental/FunctionScheduler.h>
#include <map>
#include <optional>
#include "velox/common/base/RuntimeMetrics.h"
#include "velox/common/future/VeloxPromise.h"
#include "velox/connectors/Connector.h"
Expand All @@ -42,6 +44,8 @@ class KafkaDataSource : public DataSource {
const ConnectorQueryCtx* connectorQueryCtx,
const ConnectionConfigPtr& connectionConfig);

~KafkaDataSource() override;

/// Create a kafka connection to the given topics and partitions.
void addSplit(ConnectorSplitPtr split) override;

Expand Down Expand Up @@ -98,6 +102,9 @@ class KafkaDataSource : public DataSource {
KafkaConsumerPtr consumer_;
/// The kafka record deserializer.
KafkaRecordDeserializerPtr deserializer_;
folly::FunctionScheduler scheduler_;
std::optional<ContinuePromise> blockingPromise_;
uint64_t blockingSequence_{0};
/// Count how many rows consumed.
uint64_t completedRows_ = 0;
/// Count how many bytes consumed.
Expand Down Expand Up @@ -152,6 +159,10 @@ class KafkaDataSource : public DataSource {

void applyRestoredOffsets(cppkafka::TopicPartitionList& topicPartitions);

void completeBlockingFuture();

std::optional<RowVectorPtr> blockOnPollTimeout(velox::ContinueFuture& future);

bool hasOffsetsToCommit(const TopicPartitionOffsets& offsets) const;

void updateCommittedOffsets(const TopicPartitionOffsets& offsets);
Expand Down
3 changes: 2 additions & 1 deletion velox/experimental/stateful/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ velox_add_library(
WatermarkGenerator.cpp
WatermarkSource.cpp
WatermarkAssigner.cpp
StreamRecordTimestampInserter.cpp)
StreamRecordTimestampInserter.cpp
WatermarkIdleTracker.cpp)

velox_link_libraries(
velox_stateful_exec
Expand Down
80 changes: 80 additions & 0 deletions velox/experimental/stateful/IdleTimerManager.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#pragma once

#include <atomic>
#include <cstdint>
#include <functional>
#include <memory>
#include <utility>

#include "velox/experimental/stateful/ProcessingTimeScheduler.h"

namespace facebook::velox::stateful {

// Shared one-shot idle timer helper for watermark-generating operators. It
// owns the background scheduler, prevents duplicate timer registrations, skips
// scheduling after the owner is already idle, and clamps past idle deadlines to
// the current time.
class IdleTimerManager {
public:
using TimerCallback = std::function<void(int64_t)>;

explicit IdleTimerManager(
std::unique_ptr<ProcessingTimeScheduler> scheduler = nullptr)
: scheduler_(std::move(scheduler)) {}

~IdleTimerManager() {
shutdown();
}

void schedule(
int64_t now,
bool enabled,
bool idle,
int64_t idleDeadline,
TimerCallback callback) {
if (!enabled || idle || timerPending_.exchange(true)) {
return;
}
if (!scheduler_) {
scheduler_ = std::make_unique<SystemProcessingTimeScheduler>();
}
const int64_t fireAt = idleDeadline < now ? now : idleDeadline;
scheduler_->registerTimer(
fireAt,
ProcessingTimerTask(
fireAt,
[this, callback = std::move(callback)](int64_t timestamp) mutable {
timerPending_ = false;
callback(timestamp);
}));
}

void shutdown() {
if (scheduler_) {
scheduler_->close();
scheduler_.reset();
}
timerPending_ = false;
}

private:
std::unique_ptr<ProcessingTimeScheduler> scheduler_;
std::atomic<bool> timerPending_{false};
};

} // namespace facebook::velox::stateful
6 changes: 6 additions & 0 deletions velox/experimental/stateful/StatefulOperator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ void StatefulOperator::processWatermarkStatus(bool idle) {
processWatermarkStatus(idle, 0);
}

void StatefulOperator::checkWatermarkStatus(int64_t now) {
for (auto& target : targets_) {
target->checkWatermarkStatus(now);
}
}

void StatefulOperator::initializeStateBackend(StateBackend* stateBackend) {
if (!stateHandler_) {
stateHandler_ = std::make_shared<StreamOperatorStateHandler>(
Expand Down
9 changes: 9 additions & 0 deletions velox/experimental/stateful/StatefulOperator.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class StatefulOperator {
sink = operator_->operatorType() == "TableWrite";
}

virtual ~StatefulOperator() = default;

virtual void initialize();

virtual bool isFinished();
Expand All @@ -74,6 +76,13 @@ class StatefulOperator {

virtual void processWatermarkStatus(bool idle);

/// Called periodically by the driver thread (via StatefulTask::next) after
/// each drain attempt. Operators with idle-timeout detection (e.g.
/// WatermarkAssigner) override this to check for input idleness and emit
/// WatermarkStatus events into the task output. The default implementation
/// recursively forwards the call to downstream targets.
virtual void checkWatermarkStatus(int64_t now);

virtual void initializeState();

void initializeStateBackend(StateBackend* stateBackend);
Expand Down
43 changes: 30 additions & 13 deletions velox/experimental/stateful/StatefulTask.cpp

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Blocker 1] Blocked path drops already-emitted WatermarkStatus

if (future != nullptr && future->valid()) {
  checkWatermarkStatus(...);  // may push IDLE into pendings_
  return nullptr;             // discards that pending output
}

The empty-output path below correctly re-checks pendings_ and falls through to popOutput(). The blocked path does not.

Typical failure: timer wakes mailbox → next() → source still blocked → IDLE is computed this round but returned as nullptr.

Suggestion:

if (future != nullptr && future->valid()) {
  checkWatermarkStatus(TimeWindowUtil::getCurrentProcessingTime());
  if (!pendings_.empty()) {
    return popOutput();
  }
  return nullptr;
}

Please treat this as a merge blocker and add a regression test that covers blocked + idle emission in the same next() call.

Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "velox/experimental/stateful/StatefulPlanner.h"
#include "velox/experimental/stateful/state/HashMapStateBackend.h"
#include "velox/experimental/stateful/state/RocksDBStateBackend.h"
#include "velox/experimental/stateful/window/TimeWindowUtil.h"

namespace facebook::velox::stateful {

Expand Down Expand Up @@ -141,21 +142,31 @@ StreamElementPtr StatefulTask::next(ContinueFuture* future, int32_t& retCode) {

operatorChain_->advanceWithFuture(future);
if (future != nullptr && future->valid()) {
return nullptr;
}
if (pendings_.empty()) {
if (operatorChain_->isFinished()) {
operatorChain_->finish();
finish();
// finish may trigger window flush and generate output.
// Source is blocked. Give operators a chance to detect idle input and
// emit WatermarkStatus events into pendings_ before returning.
checkWatermarkStatus(TimeWindowUtil::getCurrentProcessingTime());
if (pendings_.empty()) {
return nullptr;
}
} else {
if (pendings_.empty()) {
// No output produced. Give operators a chance to detect idle input.
checkWatermarkStatus(TimeWindowUtil::getCurrentProcessingTime());
if (pendings_.empty()) {
retCode = 1;
return nullptr;
if (operatorChain_->isFinished()) {
operatorChain_->finish();
finish();
// finish may trigger window flush and generate output.
if (pendings_.empty()) {
retCode = 1;
return nullptr;
}
} else if (operatorChain_->sourceEmpty()) {
return nullptr;
} else {
return nullptr;
}
}
} else if (operatorChain_->sourceEmpty()) {
return nullptr;
} else {
return nullptr;
}
}
return popOutput();
Expand Down Expand Up @@ -201,6 +212,12 @@ void StatefulTask::notifyWatermarkStatus(bool idle) {
operatorChain_->processWatermarkStatus(idle);
}

void StatefulTask::checkWatermarkStatus(int64_t now) {
if (operatorChain_) {
operatorChain_->checkWatermarkStatus(now);
}
}

void StatefulTask::initializeState(
const std::shared_ptr<const KeyedStateBackendParameters> parameters,
const std::vector<std::string>& checkpointRecords) {
Expand Down
4 changes: 4 additions & 0 deletions velox/experimental/stateful/StatefulTask.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ class StatefulTask : public exec::Task {

void notifyWatermarkStatus(bool idle);

/// Called on the driver thread to give each operator in the chain a chance
/// to perform idle-timeout detection and emit WatermarkStatus events.
void checkWatermarkStatus(int64_t now);

void initializeState(
const std::shared_ptr<const KeyedStateBackendParameters> params,
const std::vector<std::string>& checkpointRecords = {});
Expand Down
52 changes: 50 additions & 2 deletions velox/experimental/stateful/WatermarkAssigner.cpp

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Blocker 2] Timer scheduled from now, not from last record time

int64_t fireAt = now + idleTimeout_;

checkIdle measures idleness from lastRecordWallTime_, but the wake-up timer is anchored at the current check time. Because addInput deliberately does not re-register the timer, a late check can push fire time close to lastRecord + 2 * idleTimeout, delaying IDLE by up to ~one full timeout.

Suggestion: expose last-record (or remaining) from WatermarkIdleTracker and schedule:

fireAt = lastRecordWallTime + idleTimeout;  // clamp to now if already due

[Blocker 3] Keep scheduling idle timers after already IDLE

scheduleIdleTimer(now) runs unconditionally even when idleTracker_ is already idle (checkIdle returns false and is a no-op). After IDLE is emitted we still wake the Java mailbox every idleTimeout for no useful status change → empty next() churn.

Suggestion: skip scheduleIdleTimer when already idle (re-arm only after ACTIVE / onRecord).


[Suggestion] Calling close() from the destructor

~WatermarkAssigner() {
  WatermarkAssigner::close();
}

StatefulTask::finish() usually already called close(). Prefer a dedicated shutdownScheduler() used by both close() and the destructor, and keep business close() out of ~T(). Same pattern in ~WatermarkSource().

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "velox/experimental/stateful/WatermarkAssigner.h"

#include "velox/experimental/stateful/WatermarkGenerator.h"
#include "velox/experimental/stateful/window/TimeWindowUtil.h"

namespace facebook::velox::stateful {

Expand All @@ -26,14 +27,26 @@ WatermarkAssigner::WatermarkAssigner(
const int rowtimeFieldIndex,
const int64_t watermarkInterval)
: StatefulOperator(std::move(op), std::move(targets)),
idleTimeout_(idleTimeout),
rowtimeFieldIndex_(rowtimeFieldIndex),
watermarkInterval_(watermarkInterval) {}
watermarkInterval_(watermarkInterval),
idleTracker_(idleTimeout) {}

WatermarkAssigner::~WatermarkAssigner() {
shutdownScheduler();
}

void WatermarkAssigner::addInput(StreamElementPtr input) {
auto record = std::static_pointer_cast<StreamRecord>(input);
input_ = record->record();
op()->addInput(input_);

if (idleTracker_.isEnabled()) {
int64_t now = TimeWindowUtil::getCurrentProcessingTime();
if (idleTracker_.onRecord(now)) {
// Was idle; emit WatermarkStatus.ACTIVE downstream.
emitWatermarkStatus(false);
}
}
}

void WatermarkAssigner::advance() {
Expand Down Expand Up @@ -78,7 +91,42 @@ void WatermarkAssigner::advance() {
input_.reset();
}

void WatermarkAssigner::checkWatermarkStatus(int64_t now) {
if (!idleTracker_.isEnabled()) {
StatefulOperator::checkWatermarkStatus(now);
return;
}

if (idleTracker_.checkIdle(now)) {
emitWatermarkStatus(true);
}

scheduleIdleTimer(now);

// Forward to downstream targets.
StatefulOperator::checkWatermarkStatus(now);
}

void WatermarkAssigner::scheduleIdleTimer(int64_t now) {
idleTimerManager_.schedule(
now,
idleTracker_.isEnabled(),
idleTracker_.isIdle(),
idleTracker_.idleDeadline(),
[this](int64_t timestamp) {
auto bridge = nativeCallbackBridge();
if (bridge) {
bridge->onProcessingTime(timestamp);
}
});
}

void WatermarkAssigner::shutdownScheduler() {
idleTimerManager_.shutdown();
}

void WatermarkAssigner::close() {
shutdownScheduler();
StatefulOperator::close();
input_.reset();
}
Expand Down
Loading
Loading