Github user javadba commented on the pull request:
https://github.com/apache/spark/pull/1542#issuecomment-50814023
HI Folks,
Yes I was on master for this PR . I had realized this was not the
correct procedure so had already started to create separate branches on my fork
for the other PR's. Apologies for the scare here with the "Merge" label.
AFA this particular PR. I did work through the original (prior to my PR)
logic with a coworker. yes it is actually correct. It is also complicated. Now
I am not attempting at this point to resurrect my PR. But I will still maintain
that it is better code.
a) Shorter, more concise and yes better performing - though as Josh points
out correctly there is actually no long running code that is being locked in
the original code : so the benefits in this case are insubstantial.
b) AFA the - correct - protest about the use of interning numbers - which
could be re-used in other parts of the code. I agree. But a small fix takes
care of it. Simply prepend the interned ShuffleId with the className:objectName
and now it is unique. e.g. s"o.a.s.MapOutputTracker.shuffleID$shuffleId".
That would then not collide with potentially other usages of this tactic.
I will actually look at the half dozen other cases that use the present Set
checking tactic at some point. But we are all a bit tired of this topic right
now - and 1.1.0 is in any case "a wrap". So this particular PR can safely
remain closed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
