Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8449 )
Change subject: IMPALA-3613: Avoid topic updates to unregistered subscriber instances ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@383 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. http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc@414 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. http://gerrit.cloudera.org:8080/#/c/8449/3/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/8449/3/be/src/statestore/statestore.cc@278 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. http://gerrit.cloudera.org:8080/#/c/8449/3/be/src/statestore/statestore.cc@415 PS3, Line 415: const TUniqueId& const RegistrationId& -- To view, visit http://gerrit.cloudera.org:8080/8449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings 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 <bhara...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Sat, 04 Nov 2017 00:18:06 +0000 Gerrit-HasComments: Yes
