Fix MessageRouter hash inconsistent on C++/Java client #1029

Licht-T · 2018-01-04T11:51:56Z

Motivation

This closes #1017.

Modifications

Implement MurmurHash3 32 and use this when partitioning on C++ client.
Use MurmurHash3 32, implemented by Google Guava, on Java client.
Add hash select option.

Result

The hash inconsistent on each client are resolved.

merlimat · 2018-01-04T15:42:58Z

pulsar-client-cpp/include/pulsar/ProducerConfiguration.h

@@ -69,6 +69,9 @@ class ProducerConfiguration {
    ProducerConfiguration& setMessageRouter(const MessageRoutingPolicyPtr& router);
    const MessageRoutingPolicyPtr& getMessageRouterPtr() const;

+    ProducerConfiguration& setUseMurmurHash(const bool useMurmurHash);


I would prefer to have an enum type to select the default hashing scheme (default, since it can always be overridden in a custom message router.

Something like:

ProducerConfiguration& setDefaultHashingScheme(HashingScheme hashingScheme);

This way it might be interesting to provide the same function across C++ and Java, even the existing functions (eg: JavaStringHash)

jai1 · 2018-01-04T18:29:51Z

pulsar-client-cpp/lib/Murmur3_32.cc

@@ -0,0 +1,108 @@
+/**


@Licht-T - As per you comment in #1017

boost::hash uses MurmurHash3

Then why are we reimplementing it and changing the code on C++ side

@jai1 There are various implementations of MurmurHash3. boost::hash is much different from the Google Guava's one which is based on the original one.

Is the actual result different between Guava and boost versions?

@merlimat Yes, and the result by my implementation is same as Google Guava's.
Google Guava: https://ideone.com/Jq4stk
boost::hash: https://ideone.com/W4jEBT

Nice! Thanks for checking

sijie · 2018-01-04T20:38:49Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/SinglePartitionMessageRouterImpl.java

    }

    @Override
    public int choosePartition(Message msg) {
        // If the message has a key, it supersedes the single partition routing policy
        if (msg.hasKey()) {
-            return ((msg.getKey().hashCode() & Integer.MAX_VALUE) % numPartitions);
+            if (useMurmurHash) {
+                HashFunction hf = Hashing.murmur3_32(0);


I think this logic here is not allocation-friendly. It generates two objects HashFunction and HashCode per message here.

I would suggest using a MurmurHash class that is a static method take takes a bytes array or a string to complete a hash value, rather than using guava HashFunction or Hasher. E.g.

public class MurmurHash3 { public static int hash32(ByteBuf data, int offset, int length, int seed) { ... } }

Another suggestion - it would be good to write a jmh microbenchmark, testing the difference between using MurmurHash3 and simple hash.

@sijie I found the implementation of MurmurHash3 into Pulsar repository!
https://github.com/apache/incubator-pulsar/blob/554f0505fc424896d820d639252b28e0ec523526/pulsar-checksum/src/main/java/com/scurrilous/circe/CommonHashes.java

Licht-T · 2018-01-04T23:01:26Z

@merlimat @jai1 @sijie Thanks for your review! I will change codes.

Licht-T · 2018-01-13T14:42:06Z

I will fix after #1033 merged.

merlimat · 2018-01-26T07:25:01Z

@Licht-T This should be unblocked. I had merged all the other PRs

Licht-T · 2018-01-26T14:37:56Z

Thanks @merlimat, I'll fix up in this weekend.

Licht-T · 2018-01-26T14:38:54Z

Just fixed up C++ client. Java client will be re-implemented in the same way.

Licht-T · 2018-01-26T16:42:13Z

retest this please

merlimat

@Licht-T The C++ LGTM. Just a comment on moving the .h files into lib/ directory since they're not going to be part of user facing API.

merlimat · 2018-01-26T17:03:01Z

pulsar-client-cpp/include/pulsar/BoostHash.h

+
+#include <cstdint>
+#include <string>
+#include <boost/functional/hash.hpp>


I think this class should go in the lib/ directory since it's should not be part of the user API. As a user, I just need to know about the "Boost" hash enumerator and not its implementation

merlimat · 2018-01-26T17:04:24Z

pulsar-client-cpp/include/pulsar/Hash.h

+#include <string>
+
+namespace pulsar {
+class Hash {


Same for Hash , if the user interaction is with the enum, no need to expose this interface. In any case the user won't be able to directly supply a different hash function (other than writing a custom router).

merlimat · 2018-01-27T17:22:31Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ProducerConfiguration.java

@@ -46,7 +46,7 @@
    private boolean blockIfQueueFull = false;
    private int maxPendingMessages = 1000;
    private MessageRoutingMode messageRouteMode = MessageRoutingMode.SinglePartition;
-    private boolean useMurmurHash = true;
+    private HashingScheme hashingScheme = HashingScheme.Murmur3_32Hash;


We cannot change the default from Java to Murmur hash, since it would unexpectedly break ordering for people upgrading the library.

@merlimat Is this also can be said to C++ client?

Yes, unfortunately we cannot change the default in either library implementation. C++ would have to keep using the current BoostHash

merlimat · 2018-01-27T17:24:34Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageRouterBase.java

+    MessageRouterBase(ProducerConfiguration.HashingScheme hashingScheme) {
+        switch (hashingScheme) {
+            case JavaStringHash:
+                this.hash = new JavaStringHash();


Maybe we can keep static instances of the Hashing objects since they can be shared around

merlimat · 2018-01-27T17:26:07Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/Hash.java

+package org.apache.pulsar.client.impl;
+
+public interface Hash {
+    long makeHash(String s);


Since we're converting later to an int, we should just have an it here.

@merlimat MurmurHash returns an unsigned int value, so we have to return as a positive value for calculating the partition.

Sure, but the number is anyway masked with & Integer.MAX_INT so the bit sign will be stripped.

@merlimat Does this conversion keep the quality of hash?

I think this is not good workaround because this may waste the quality of hash.

I don't think that it really changes anything because the hash is then moduled to the number of partitions, so all the high order bits are anyway discarded.

@merlimat IMO, this conversion may break the uniform distribution of hash. I concern that message routing is biased to the specific partition.

@merlimat After second thought, I found you are right! Excuse me!

merlimat · 2018-01-27T17:27:32Z

...client/src/main/java/org/apache/pulsar/client/impl/RoundRobinPartitionMessageRouterImpl.java

-            else {
-                return ((msg.getKey().hashCode() & Integer.MAX_VALUE) % topicMetadata.numPartitions());
-            }
+            return (int) (hash.makeHash(msg.getKey()) % topicMetadata.numPartitions());


This depends on the makeHash() to always return a positive number, which is true in the current implementation. We should mention that in the makeHash() javadoc.

merlimat · 2018-01-27T22:38:38Z

@Licht-T The change looks good to me. Just the last thing is to move the new headers added in pulsar-client-cpp/include/pulsar/BoostHash.h into pulsar-client-cpp/lib/... because they should not be exposed as public API to the user.

Licht-T · 2018-01-28T09:02:26Z

retest this please

Licht-T · 2018-01-29T00:40:21Z

@merlimat C++ client is now fixed. I'll add some tests.

merlimat

👍 Nice change!

…integer This is the same behavior as Hash classes on Java client

Licht-T · 2018-01-30T21:26:13Z

@merlimat Now tests added!

saandrews · 2018-02-02T07:32:33Z

@Licht-T Some of the changes like "constexpr" seems to be c++11 specific and it's breaking our build.

Licht-T · 2018-02-02T07:36:32Z

@saandrews Oops! Excuse me! I'll fix soon!

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Jan 4, 2018

merlimat added this to the 1.22.0-incubating milestone Jan 4, 2018

merlimat assigned Licht-T Jan 4, 2018

merlimat reviewed Jan 4, 2018

View reviewed changes

merlimat requested review from rdhabalia, jai1, saandrews, nkurihar and yush1ga January 4, 2018 18:23

jai1 reviewed Jan 4, 2018

View reviewed changes

sijie reviewed Jan 4, 2018

View reviewed changes

This was referenced Jan 5, 2018

Issue 1024: Message Router should take numPartitions as parameter in choosing partition #1025

Merged

ENH: Add C++ code auto-formatting option #1038

Merged

Fix hash inconsistent on between C++ and Java clients.

10ff43b

Licht-T force-pushed the fix-hash-inconsistent branch from 87de4d5 to 10ff43b Compare January 26, 2018 14:36

merlimat reviewed Jan 26, 2018

View reviewed changes

Add HashingScheme to select hash function on Java client

31b26ab

Licht-T force-pushed the fix-hash-inconsistent branch from 11ec26d to 31b26ab Compare January 27, 2018 17:05

Fix the bug of Murmur3_32Hash on C++ client

14f902b

merlimat reviewed Jan 27, 2018

View reviewed changes

Licht-T added 4 commits January 28, 2018 03:23

Add Javadoc on makeHash method

0e156b6

Use JavaStringHash as default hash on Java client

7f90a37

Use BoostHash as default hash on C++ client

699a03b

Make hash method always returns a signed integer

2dfa743

Licht-T force-pushed the fix-hash-inconsistent branch from 3b16f8b to 4cb3d88 Compare January 28, 2018 03:10

Re-implement hash classes as singleton on Java client

d4a0e9c

Licht-T force-pushed the fix-hash-inconsistent branch from 4cb3d88 to d4a0e9c Compare January 28, 2018 03:11

Licht-T added 4 commits January 28, 2018 22:22

Move hash classes from include to lib

6bee3b8

Change constructor argument of hash classes

f62ff01

Remove unused headers

5f1006f

Remove auto type

08a74c1

merlimat approved these changes Jan 29, 2018

View reviewed changes

Licht-T added 2 commits January 31, 2018 06:03

Fix C++ client Hash classes so that these return non-negative signed …

a1ebdb3

…integer This is the same behavior as Hash classes on Java client

Add tests for C++/Java client Hash

88f9d4d

merlimat merged commit 8d159ef into apache:master Jan 31, 2018

Licht-T mentioned this pull request Feb 5, 2018

Remove constexpr from C++ client #1182

Merged

Fix MessageRouter hash inconsistent on C++/Java client #1029

Fix MessageRouter hash inconsistent on C++/Java client #1029

Conversation

Licht-T commented Jan 4, 2018 • edited Loading

Motivation

Modifications

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Licht-T Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Licht-T commented Jan 4, 2018

Licht-T commented Jan 13, 2018

merlimat commented Jan 26, 2018

Licht-T commented Jan 26, 2018

Licht-T commented Jan 26, 2018 • edited Loading

Licht-T commented Jan 26, 2018

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Licht-T Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Licht-T Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Jan 27, 2018

Licht-T commented Jan 28, 2018

Licht-T commented Jan 29, 2018

merlimat left a comment

Choose a reason for hiding this comment

Licht-T commented Jan 30, 2018

saandrews commented Feb 2, 2018

Licht-T commented Feb 2, 2018

Licht-T commented Jan 4, 2018 •

edited

Loading

Licht-T Jan 4, 2018 •

edited

Loading

Licht-T commented Jan 26, 2018 •

edited

Loading

Licht-T Jan 27, 2018 •

edited

Loading

merlimat Jan 27, 2018 •

edited

Loading

Licht-T Jan 27, 2018 •

edited

Loading