Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue #21 #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 44 additions & 4 deletions ndn-svs/svspubsub.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,24 @@ SVSPubSub::publish(const Name& name, span<const uint8_t> value,
}
}


SeqNo
SVSPubSub::publish(const Name& name,
const Name& nodePrefix, time::milliseconds freshnessPeriod,
std::vector<Block> mappingBlocks)
{
// Segment the data if larger than MAX_DATA_SIZE

NodeID nid = nodePrefix == EMPTY_NAME ? m_dataPrefix : nodePrefix;
SeqNo seqNo = m_svsync.getCore().getSeqNo(nid) + 1;

// Insert mapping and manually update the sequence number
insertMapping(nid, seqNo, name, mappingBlocks);
m_svsync.getCore().updateSeqNo(seqNo, nid);

return seqNo;
Copy link
Member

Choose a reason for hiding this comment

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

indentation looks wrong

}

SeqNo
SVSPubSub::publishPacket(const Data& data, const Name& nodePrefix,
std::vector<Block> mappingBlocks)
Expand Down Expand Up @@ -140,10 +158,10 @@ SVSPubSub::subscribe(const Name& prefix, const SubscriptionCallback& callback, b
}

uint32_t
SVSPubSub::subscribeWithRegex(const Regex &regex, const SubscriptionCallback &callback, bool packets)
SVSPubSub::subscribeWithRegex(const Regex &regex, const SubscriptionCallback &callback,bool autofetch, bool packets)
{
uint32_t handle = ++m_subscriptionCount;
Subscription sub = { handle, ndn::Name(), callback, packets, false, make_shared<Regex>(regex)};
Subscription sub = { handle, ndn::Name(), callback, packets, false, make_shared<Regex>(regex), autofetch};
m_regexSubscriptions.push_back(sub);
return handle;
}
Expand Down Expand Up @@ -284,19 +302,41 @@ SVSPubSub::processMapping(const NodeID& nodeId, SeqNo seqNo)
bool queued = false;
for (const auto& sub : m_prefixSubscriptions)
{
if (sub.prefix.isPrefixOf(mapping.first))
if (sub.prefix.isPrefixOf(mapping.first) and sub.autofetch)
Copy link
Member

Choose a reason for hiding this comment

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

please use && in C++

{
m_fetchMap[std::pair(nodeId, seqNo)].push_back(sub);
queued = true;
}
else if (sub.prefix.isPrefixOf(mapping.first) and !sub.autofetch)
Copy link
Member

Choose a reason for hiding this comment

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

refactor the if/else to avoid repeating the conditions

{
SubscriptionData subData = {
mapping.first,
ndn::span<const uint8_t>{},
nodeId,
seqNo,
ndn::Data()
Copy link
Member

Choose a reason for hiding this comment

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

I don't love this "passing an empty Data" to be honest...

Copy link
Author

Choose a reason for hiding this comment

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

Do you have any suggestion? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Not at the moment... I didn't do too much thinking to be honest. But maybe this should be a new/separate API instead of shoehorning it into the existing API with a bool as discriminator.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah... But it's better to be compatible with old APIs because of existing apps...

};
sub.callback(subData);
}
}
for (const auto& sub : m_regexSubscriptions)
{
if (sub.regex->match(mapping.first))
if (sub.regex->match(mapping.first) and sub.autofetch)
Copy link
Member

Choose a reason for hiding this comment

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

...&&...

{
m_fetchMap[std::pair(nodeId, seqNo)].push_back(sub);
queued = true;
}
else if (sub.regex->match(mapping.first) and !sub.autofetch)
{
SubscriptionData subData = {
mapping.first,
ndn::span<const uint8_t>{},
nodeId,
seqNo,
ndn::Data()
};
sub.callback(subData);
}
}

return queued;
Expand Down
17 changes: 16 additions & 1 deletion ndn-svs/svspubsub.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,20 @@ class SVSPubSub : noncopyable
time::milliseconds freshnessPeriod = FRESH_FOREVER,
std::vector<Block> mappingBlocks = {});

/**
* @brief Publish data names only on the pub/sub group.
*
* @param name name for the publication
* @param nodePrefix Name to publish the data under
* @param freshnessPeriod freshness period for the data
* @param mappingBlocks Additional blocks to be published with the mapping (use sparingly)
*/
SeqNo
publish(const Name& name,
const Name& nodePrefix = EMPTY_NAME,
time::milliseconds freshnessPeriod = FRESH_FOREVER,
std::vector<Block> mappingBlocks = {});

/**
* @brief Subscribe to a application name prefix.
*
Expand All @@ -138,7 +152,7 @@ class SVSPubSub : noncopyable
* @returns Handle to the subscription
*/
uint32_t
subscribeWithRegex(const Regex& regex, const SubscriptionCallback& callback, bool packets = false);
subscribeWithRegex(const Regex& regex, const SubscriptionCallback& callback, bool autofetch = true, bool packets = false);

/**
* @brief Subscribe to a data producer
Expand Down Expand Up @@ -194,6 +208,7 @@ class SVSPubSub : noncopyable
bool isPacketSubscription;
bool prefetch;
std::shared_ptr<Regex> regex = make_shared<Regex>("^<>+$");
Copy link
Member

Choose a reason for hiding this comment

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

why a shared_ptr?

bool autofetch = true;
Copy link
Member

Choose a reason for hiding this comment

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

This wastes space in the struct, please move the bool next to the others above to minimize padding. Even better, the bools and the uint32_t should be grouped together.

};

void
Expand Down