Skip to content

Commit 916af95

Browse files
BewareMyPowerBewareMyPower
committed
Merge branch 'main' into branch-3.5
2 parents 3b574c7 + ee1d7b9 commit 916af95

14 files changed

+183
-55
lines changed

‎.github/workflows/ci-pr-validation.yaml‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,20 @@ concurrency:
2929

3030
jobs:
3131

32+
formatting-check:
33+
name: Formatting Check
34+
runs-on: ubuntu-latest
35+
steps:
36+
- uses: actions/checkout@v3
37+
- name: Run clang-format style check for C/C++/Protobuf programs.
38+
uses: jidicula/clang-format-action@v4.11.0
39+
with:
40+
clang-format-version: '11'
41+
exclude-regex: '.*\.(proto|hpp)'
42+
3243
wireshark-dissector-build:
3344
name: Build the Wireshark dissector
45+
needs: formatting-check
3446
runs-on: ${{ matrix.os }}
3547
timeout-minutes: 60
3648
strategy:
@@ -61,6 +73,7 @@ jobs:
6173
6274
unit-tests:
6375
name: Run unit tests
76+
needs: formatting-check
6477
runs-on: ubuntu-22.04
6578
timeout-minutes: 120
6679

@@ -81,8 +94,8 @@ jobs:
8194
./vcpkg/vcpkg format-manifest vcpkg.json
8295
if [[ $(git diff | wc -l) -gt 0 ]]; then
8396
echo "Please run `./vcpkg/vcpkg format-manifest vcpkg.json` to reformat vcpkg.json"
97+
exit 1
8498
fi
85-
make check-format
8699
87100
- name: Build tests
88101
run: |
@@ -103,6 +116,7 @@ jobs:
103116
104117
cpp20-build:
105118
name: Build with the C++20 standard
119+
needs: formatting-check
106120
runs-on: ubuntu-22.04
107121
timeout-minutes: 60
108122

@@ -288,8 +302,8 @@ jobs:
288302
cpp-build-macos:
289303
timeout-minutes: 120
290304
name: Build CPP Client on macOS
305+
needs: formatting-check
291306
runs-on: macos-12
292-
needs: unit-tests
293307
steps:
294308
- name: checkout
295309
uses: actions/checkout@v3
@@ -306,6 +320,12 @@ jobs:
306320
run: |
307321
cmake --build ./build-macos --parallel --config Release
308322
323+
- name: Build with C++20
324+
shell: bash
325+
run: |
326+
cmake -B build-macos-cpp20 -DCMAKE_CXX_STANDARD=20
327+
cmake --build build-macos-cpp20 -j8
328+
309329
cpp-build-macos-static:
310330
timeout-minutes: 120
311331
name: Build CPP Client on macOS with static dependencies
@@ -332,7 +352,7 @@ jobs:
332352
check-completion:
333353
name: Check Completion
334354
runs-on: ubuntu-latest
335-
needs: [wireshark-dissector-build, unit-tests, cpp20-build, cpp-build-windows, package, cpp-build-macos]
355+
needs: [formatting-check, wireshark-dissector-build, unit-tests, cpp20-build, cpp-build-windows, package, cpp-build-macos]
336356

337357
steps:
338358
- run: true

‎lib/ClientConnection.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1321,7 +1321,7 @@ void ClientConnection::close(Result result, bool detach) {
13211321
}
13221322
// Remove the connection from the pool before completing any promise
13231323
if (detach) {
1324-
pool_.remove(logicalAddress_ + "-" + std::to_string(poolIndex_), this);
1324+
pool_.remove(logicalAddress_, physicalAddress_, poolIndex_, this);
13251325
}
13261326

13271327
auto self = shared_from_this();

‎lib/ConnectionPool.cc‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,13 @@ bool ConnectionPool::close() {
6666
return true;
6767
}
6868

69+
static const std::string getKey(const std::string& logicalAddress, const std::string& physicalAddress,
70+
size_t keySuffix) {
71+
std::stringstream ss;
72+
ss << logicalAddress << '-' << physicalAddress << '-' << keySuffix;
73+
return ss.str();
74+
}
75+
6976
Future<Result, ClientConnectionWeakPtr> ConnectionPool::getConnectionAsync(const std::string& logicalAddress,
7077
const std::string& physicalAddress,
7178
size_t keySuffix) {
@@ -77,9 +84,7 @@ Future<Result, ClientConnectionWeakPtr> ConnectionPool::getConnectionAsync(const
7784

7885
std::unique_lock<std::recursive_mutex> lock(mutex_);
7986

80-
std::stringstream ss;
81-
ss << logicalAddress << '-' << keySuffix;
82-
const std::string key = ss.str();
87+
auto key = getKey(logicalAddress, physicalAddress, keySuffix);
8388

8489
PoolMap::iterator cnxIt = pool_.find(key);
8590
if (cnxIt != pool_.end()) {
@@ -127,7 +132,9 @@ Future<Result, ClientConnectionWeakPtr> ConnectionPool::getConnectionAsync(const
127132
return future;
128133
}
129134

130-
void ConnectionPool::remove(const std::string& key, ClientConnection* value) {
135+
void ConnectionPool::remove(const std::string& logicalAddress, const std::string& physicalAddress,
136+
size_t keySuffix, ClientConnection* value) {
137+
auto key = getKey(logicalAddress, physicalAddress, keySuffix);
131138
std::lock_guard<std::recursive_mutex> lock(mutex_);
132139
auto it = pool_.find(key);
133140
if (it != pool_.end() && it->second.get() == value) {

‎lib/ConnectionPool.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ class PULSAR_PUBLIC ConnectionPool {
5151
*/
5252
bool close();
5353

54-
void remove(const std::string& key, ClientConnection* value);
54+
void remove(const std::string& logicalAddress, const std::string& physicalAddress, size_t keySuffix,
55+
ClientConnection* value);
5556

5657
/**
5758
* Get a connection from the pool.

‎lib/ConsumerImpl.cc‎

Lines changed: 50 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -236,16 +236,14 @@ Future<Result, bool> ConsumerImpl::connectionOpened(const ClientConnectionPtr& c
236236
// sending the subscribe request.
237237
cnx->registerConsumer(consumerId_, get_shared_this_ptr());
238238

239-
if (duringSeek_) {
239+
if (duringSeek()) {
240240
ackGroupingTrackerPtr_->flushAndClean();
241241
}
242242

243243
Lock lockForMessageId(mutexForMessageId_);
244-
// Update startMessageId so that we can discard messages after delivery restarts
245-
const auto startMessageId = clearReceiveQueue();
244+
clearReceiveQueue();
246245
const auto subscribeMessageId =
247-
(subscriptionMode_ == Commands::SubscriptionModeNonDurable) ? startMessageId : boost::none;
248-
startMessageId_ = startMessageId;
246+
(subscriptionMode_ == Commands::SubscriptionModeNonDurable) ? startMessageId_.get() : boost::none;
249247
lockForMessageId.unlock();
250248

251249
unAckedMessageTrackerPtr_->clear();
@@ -1048,14 +1046,21 @@ void ConsumerImpl::messageProcessed(Message& msg, bool track) {
10481046
* Clear the internal receiver queue and returns the message id of what was the 1st message in the queue that
10491047
* was
10501048
* not seen by the application
1049+
* `startMessageId_` is updated so that we can discard messages after delivery restarts.
10511050
*/
1052-
boost::optional<MessageId> ConsumerImpl::clearReceiveQueue() {
1053-
bool expectedDuringSeek = true;
1054-
if (duringSeek_.compare_exchange_strong(expectedDuringSeek, false)) {
1055-
return seekMessageId_.get();
1051+
void ConsumerImpl::clearReceiveQueue() {
1052+
if (duringSeek()) {
1053+
startMessageId_ = seekMessageId_.get();
1054+
SeekStatus expected = SeekStatus::COMPLETED;
1055+
if (seekStatus_.compare_exchange_strong(expected, SeekStatus::NOT_STARTED)) {
1056+
auto seekCallback = seekCallback_.release();
1057+
executor_->postWork([seekCallback] { seekCallback(ResultOk); });
1058+
}
1059+
return;
10561060
} else if (subscriptionMode_ == Commands::SubscriptionModeDurable) {
1057-
return startMessageId_.get();
1061+
return;
10581062
}
1063+
10591064
Message nextMessageInQueue;
10601065
if (incomingMessages_.peekAndClear(nextMessageInQueue)) {
10611066
// There was at least one message pending in the queue
@@ -1071,16 +1076,12 @@ boost::optional<MessageId> ConsumerImpl::clearReceiveQueue() {
10711076
.ledgerId(nextMessageId.ledgerId())
10721077
.entryId(nextMessageId.entryId() - 1)
10731078
.build();
1074-
return previousMessageId;
1079+
startMessageId_ = previousMessageId;
10751080
} else if (lastDequedMessageId_ != MessageId::earliest()) {
10761081
// If the queue was empty we need to restart from the message just after the last one that has been
10771082
// dequeued
10781083
// in the past
1079-
return lastDequedMessageId_;
1080-
} else {
1081-
// No message was received or dequeued by this consumer. Next message would still be the
1082-
// startMessageId
1083-
return startMessageId_.get();
1084+
startMessageId_ = lastDequedMessageId_;
10841085
}
10851086
}
10861087

@@ -1500,18 +1501,15 @@ void ConsumerImpl::seekAsync(uint64_t timestamp, ResultCallback callback) {
15001501

15011502
bool ConsumerImpl::isReadCompacted() { return readCompacted_; }
15021503

1503-
inline bool hasMoreMessages(const MessageId& lastMessageIdInBroker, const MessageId& messageId) {
1504-
return lastMessageIdInBroker > messageId && lastMessageIdInBroker.entryId() != -1;
1505-
}
1506-
15071504
void ConsumerImpl::hasMessageAvailableAsync(HasMessageAvailableCallback callback) {
1508-
const auto startMessageId = startMessageId_.get();
1509-
Lock lock(mutexForMessageId_);
1510-
const auto messageId =
1511-
(lastDequedMessageId_ == MessageId::earliest()) ? startMessageId.value() : lastDequedMessageId_;
1512-
1513-
if (messageId == MessageId::latest()) {
1514-
lock.unlock();
1505+
bool compareMarkDeletePosition;
1506+
{
1507+
std::lock_guard<std::mutex> lock{mutexForMessageId_};
1508+
compareMarkDeletePosition =
1509+
(lastDequedMessageId_ == MessageId::earliest()) &&
1510+
(startMessageId_.get().value_or(MessageId::earliest()) == MessageId::latest());
1511+
}
1512+
if (compareMarkDeletePosition) {
15151513
auto self = get_shared_this_ptr();
15161514
getLastMessageIdAsync([self, callback](Result result, const GetLastMessageIdResponse& response) {
15171515
if (result != ResultOk) {
@@ -1543,16 +1541,15 @@ void ConsumerImpl::hasMessageAvailableAsync(HasMessageAvailableCallback callback
15431541
}
15441542
});
15451543
} else {
1546-
if (hasMoreMessages(lastMessageIdInBroker_, messageId)) {
1547-
lock.unlock();
1544+
if (hasMoreMessages()) {
15481545
callback(ResultOk, true);
15491546
return;
15501547
}
1551-
lock.unlock();
1552-
1553-
getLastMessageIdAsync([callback, messageId](Result result, const GetLastMessageIdResponse& response) {
1554-
callback(result, (result == ResultOk) && hasMoreMessages(response.getLastMessageId(), messageId));
1555-
});
1548+
auto self = get_shared_this_ptr();
1549+
getLastMessageIdAsync(
1550+
[this, self, callback](Result result, const GetLastMessageIdResponse& response) {
1551+
callback(result, (result == ResultOk) && hasMoreMessages());
1552+
});
15561553
}
15571554
}
15581555

@@ -1656,9 +1653,18 @@ void ConsumerImpl::seekAsyncInternal(long requestId, SharedBuffer seek, const Me
16561653
return;
16571654
}
16581655

1656+
auto expected = SeekStatus::NOT_STARTED;
1657+
if (!seekStatus_.compare_exchange_strong(expected, SeekStatus::IN_PROGRESS)) {
1658+
LOG_ERROR(getName() << " attempted to seek (" << seekId << ", " << timestamp << " when the status is "
1659+
<< static_cast<int>(expected));
1660+
callback(ResultNotAllowedError);
1661+
return;
1662+
}
1663+
16591664
const auto originalSeekMessageId = seekMessageId_.get();
16601665
seekMessageId_ = seekId;
1661-
duringSeek_ = true;
1666+
seekStatus_ = SeekStatus::IN_PROGRESS;
1667+
seekCallback_ = std::move(callback);
16621668
if (timestamp > 0) {
16631669
LOG_INFO(getName() << " Seeking subscription to " << timestamp);
16641670
} else {
@@ -1682,12 +1688,19 @@ void ConsumerImpl::seekAsyncInternal(long requestId, SharedBuffer seek, const Me
16821688
Lock lock(mutexForMessageId_);
16831689
lastDequedMessageId_ = MessageId::earliest();
16841690
lock.unlock();
1691+
if (getCnx().expired()) {
1692+
// It's during reconnection, complete the seek future after connection is established
1693+
seekStatus_ = SeekStatus::COMPLETED;
1694+
} else {
1695+
startMessageId_ = seekMessageId_.get();
1696+
seekCallback_.release()(result);
1697+
}
16851698
} else {
16861699
LOG_ERROR(getName() << "Failed to seek: " << result);
16871700
seekMessageId_ = originalSeekMessageId;
1688-
duringSeek_ = false;
1701+
seekStatus_ = SeekStatus::NOT_STARTED;
1702+
seekCallback_.release()(result);
16891703
}
1690-
callback(result);
16911704
});
16921705
}
16931706

‎lib/ConsumerImpl.h‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,13 @@ const static std::string SYSTEM_PROPERTY_REAL_TOPIC = "REAL_TOPIC";
7575
const static std::string PROPERTY_ORIGIN_MESSAGE_ID = "ORIGIN_MESSAGE_ID";
7676
const static std::string DLQ_GROUP_TOPIC_SUFFIX = "-DLQ";
7777

78+
enum class SeekStatus : std::uint8_t
79+
{
80+
NOT_STARTED,
81+
IN_PROGRESS,
82+
COMPLETED
83+
};
84+
7885
class ConsumerImpl : public ConsumerImplBase {
7986
public:
8087
ConsumerImpl(const ClientImplPtr client, const std::string& topic, const std::string& subscriptionName,
@@ -193,7 +200,7 @@ class ConsumerImpl : public ConsumerImplBase {
193200
const DeadlineTimerPtr& timer,
194201
BrokerGetLastMessageIdCallback callback);
195202

196-
boost::optional<MessageId> clearReceiveQueue();
203+
void clearReceiveQueue();
197204
void seekAsyncInternal(long requestId, SharedBuffer seek, const MessageId& seekId, long timestamp,
198205
ResultCallback callback);
199206
void processPossibleToDLQ(const MessageId& messageId, ProcessDLQCallBack cb);
@@ -239,10 +246,13 @@ class ConsumerImpl : public ConsumerImplBase {
239246
MessageId lastDequedMessageId_{MessageId::earliest()};
240247
MessageId lastMessageIdInBroker_{MessageId::earliest()};
241248

242-
std::atomic_bool duringSeek_{false};
249+
std::atomic<SeekStatus> seekStatus_{SeekStatus::NOT_STARTED};
250+
Synchronized<ResultCallback> seekCallback_{[](Result) {}};
243251
Synchronized<boost::optional<MessageId>> startMessageId_;
244252
Synchronized<MessageId> seekMessageId_{MessageId::earliest()};
245253

254+
bool duringSeek() const { return seekStatus_ != SeekStatus::NOT_STARTED; }
255+
246256
class ChunkedMessageCtx {
247257
public:
248258
ChunkedMessageCtx() : totalChunks_(0) {}
@@ -332,6 +342,23 @@ class ConsumerImpl : public ConsumerImplBase {
332342
const proto::MessageIdData& messageIdData,
333343
const ClientConnectionPtr& cnx, MessageId& messageId);
334344

345+
bool hasMoreMessages() const {
346+
std::lock_guard<std::mutex> lock{mutexForMessageId_};
347+
if (lastMessageIdInBroker_.entryId() == -1L) {
348+
return false;
349+
}
350+
351+
const auto inclusive = config_.isStartMessageIdInclusive();
352+
if (lastDequedMessageId_ == MessageId::earliest()) {
353+
// If startMessageId_ is none, use latest so that this method will return false
354+
const auto startMessageId = startMessageId_.get().value_or(MessageId::latest());
355+
return inclusive ? (lastMessageIdInBroker_ >= startMessageId)
356+
: (lastMessageIdInBroker_ > startMessageId);
357+
} else {
358+
return lastMessageIdInBroker_ > lastDequedMessageId_;
359+
}
360+
}
361+
335362
friend class PulsarFriend;
336363
friend class MultiTopicsConsumerImpl;
337364

‎lib/MultiTopicsConsumerImpl.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ void MultiTopicsConsumerImpl::closeAsync(ResultCallback originalCallback) {
475475
};
476476
const auto state = state_.load();
477477
if (state == Closing || state == Closed) {
478-
callback(ResultAlreadyClosed);
478+
callback(ResultOk);
479479
return;
480480
}
481481

@@ -488,7 +488,7 @@ void MultiTopicsConsumerImpl::closeAsync(ResultCallback originalCallback) {
488488
if (consumers.empty()) {
489489
LOG_DEBUG("TopicsConsumer have no consumers to close "
490490
<< " topic" << topic() << " subscription - " << subscriptionName_);
491-
callback(ResultAlreadyClosed);
491+
callback(ResultOk);
492492
return;
493493
}
494494

‎lib/NamespaceName.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ std::shared_ptr<NamespaceName> NamespaceName::getNamespaceObject() {
9393
return std::shared_ptr<NamespaceName>(this);
9494
}
9595

96-
bool NamespaceName::operator==(const NamespaceName& namespaceName) {
96+
bool NamespaceName::operator==(const NamespaceName& namespaceName) const {
9797
return this->namespace_.compare(namespaceName.namespace_) == 0;
9898
}
9999

‎lib/NamespaceName.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ class PULSAR_PUBLIC NamespaceName : public ServiceUnitId {
3737
static std::shared_ptr<NamespaceName> get(const std::string& property, const std::string& cluster,
3838
const std::string& namespaceName);
3939
static std::shared_ptr<NamespaceName> get(const std::string& property, const std::string& namespaceName);
40-
bool operator==(const NamespaceName& namespaceName);
40+
bool operator==(const NamespaceName& namespaceName) const;
4141
bool isV2();
4242
std::string toString();
4343

0 commit comments

Comments
 (0)