krasserm / akka-persistence-kafka

A replicated Akka Persistence journal backed by Apache Kafka
Apache License 2.0
201 stars 59 forks source link

Problem with persistenceId values using "illegal" characters #5

Closed tsandall closed 10 years ago

tsandall commented 10 years ago

Ran into this when trying to use cluster sharding from the contrib module. The ShardCoordinator uses it's actor path as the persistenceId. As a result the persistenceId value contains slash characters.

This is the error seen in the Kafka server logs: https://gist.github.com/tsandall/07610566f8aa5547ad77

Any ideas on how to resolve this?

krasserm commented 10 years ago

This needs to be solved by properly encoding topic names. Probably keeping alpha-numeric characters, ., - and _ whereas replacing any other character with _ sounds like a reasonable default to me. Please let me know if you see any issues with that. I'll try to push a fix soon. Thanks for reporting.

tsandall commented 10 years ago

I ended up working around the problem by encoding the persistence IDs using a modified base64 ('_', '-' instead of '/', '+'). Either solution sounds fine to me. If you want I can submit a PR.

https://github.com/tsandall/akka-persistence-kafka/commit/19a8b4dd5e1351983160aca7cddcd114691b4be9

krasserm commented 10 years ago

Thanks for offering help but I'm already hacking on it, sorry for having not asked you before. This also needs changes to the snapshot plugin. I'm also about to extend the tests.

tsandall commented 10 years ago

No problem. I'll take whatever you push. Thanks for the quick response.

krasserm commented 10 years ago

@tsandall can you please give the latest version in master a try? Thanks!

tsandall commented 10 years ago

Looks fine to me. Tried it out locally and didn't notice any problems. I can see the translated IDs in ZK:

/brokers/topics/_user_sharding_accountCoordinator_singleton_coordinator
{ u'partitions': { u'0': [1]}, u'version': 1}

/brokers/topics/_user_sharding_accountCoordinator_singleton_coordinator/partitions/0/state
{ u'controller_epoch': 1,
  u'isr': [1],
  u'leader': 1,
  u'leader_epoch': 0,
  u'version': 1}
krasserm commented 10 years ago

Glad that it works, thanks again. Cheers!