impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3613: Avoid topic updates to unregistered subscriber instances
Date Sat, 04 Nov 2017 00:18:06 GMT
Sailesh Mukil has posted comments on this change. ( )

Change subject: IMPALA-3613: Avoid topic updates to unregistered subscriber instances

Patch Set 3:

File be/src/statestore/statestore.h:
PS2, Line 383: d subscriber
> >  Where ever the 'SubscriberId' is
Thanks for the explanation.

Yea my point was if we're going to have a unique RegistrationId anyway, why have a SubscriberId.
It seemed redundant.

But as you pointed out, it looks like the subscriber chooses the subscriber_id and not the
statestore. So, it would be hard to enforce this.

Let's leave this for now.
File be/src/statestore/
PS2, Line 414: onst SubscriberId& subscriber_id,
             :     const TUniqueId& registration_id
> Not sure I understand. We get the subscriber/registration_id from the Sched
Nvm, my bad, I thought both were coming from the Subscriber object. Ignore this.
File be/src/statestore/
PS3, Line 278:   lock_guard<SpinLock> l(subscribers_lock_);
             :   lock_guard<mutex> t(topic_lock_);
I just noticed this. Getting a SpinLock before getting a mutex is an anti-pattern.

Even attempting to get a spinlock while already holding a spinlock is also not exactly a great
idea. However, our SpinLock implementation sleeps after a few cycles of trying to obtain the
lock anyway.

Do we know if we do a lot of work holding the topic_lock_? If not, let's change this to a
SpinLock too. (The GatherTopicUpdates() holds topic_lock_ and iterates through a nested loop,
but I'm not sure how many iterations that would be in the worst case).

If it looks like we will end up doing a lot of work holing the lock, we can be safe and just
turn the 'subscribers_lock_' back to a mutex.
PS3, Line 415: const TUniqueId&
const RegistrationId&

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0329ae7d23dc6e9b04b7bc3ee8d89cbc73756f65
Gerrit-Change-Number: 8449
Gerrit-PatchSet: 3
Gerrit-Owner: Bharath Vissapragada <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Sat, 04 Nov 2017 00:18:06 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message