Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-188634249
Just did another merge to keep it up to date with master branch. Not sure
what the plan is now. I would be happy to make any changes that can help to
make the final
Github user AwesomeJohnR commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-187379599
Any updates on the status of this merge? This update would be very helpful
---
If your project is set up for it, you can reply to this email and have your
reply app
Github user choang commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-169555291
this looks good enough
---
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 f
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-169487103
@choang changes are made and latest code from master merged in. Please
review whenever you have time. -thanks
---
If your project is set up for it, you can reply t
Github user choang commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-147967734
`StateStore` implementations could be designed like:
```
public class KafkaStateStore implements StateStore {
public KafkaStateStore(HostPort kafkaBroker, St
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-147937919
@choang In recent changes, I have made it possible to plug in custom store
implementations. The custom implementation is given the opportunity to
initialize itself b
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-146006292
@choang thanks for the code review. Your comment on sharing the kafka store
for all partitions is really helpful and I was able to cleanup a lot of
unnecessary logic.
Github user choang commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-146005945
looks really good. just had a few nits that shouldn't block. I also don't
have a vote, so you'll need to find a proper reviewer :)
---
If your project is set up for it,
Github user choang commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r41324097
--- Diff: external/storm-kafka/src/jvm/storm/kafka/ZkStateStore.java ---
@@ -54,50 +56,76 @@ public CuratorFramework getCurator() {
return _curator;
Github user choang commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r41323806
--- Diff: external/storm-kafka/src/jvm/storm/kafka/PartitionManager.java ---
@@ -252,28 +253,26 @@ public void fail(Long offset) {
public void commit()
Github user choang commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r41323220
--- Diff: external/storm-kafka/src/jvm/storm/kafka/KafkaStateStore.java ---
@@ -0,0 +1,220 @@
+package storm.kafka;
+
+import com.google.common.colle
Github user choang commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r41186998
--- Diff: external/storm-kafka/src/jvm/storm/kafka/KafkaSpout.java ---
@@ -43,19 +42,19 @@ public MessageAndRealOffset(Message msg, long offset) {
}
Github user choang commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r41186043
--- Diff: external/storm-kafka/src/jvm/storm/kafka/StateStore.java ---
@@ -0,0 +1,43 @@
+package storm.kafka;
+
+import java.util.Map;
+
+/**
Github user choang commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r41185586
--- Diff: external/storm-kafka/src/jvm/storm/kafka/KafkaDataStore.java ---
@@ -0,0 +1,219 @@
+package storm.kafka;
+
+import com.google.common.collec
Github user choang commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r41185141
--- Diff: external/storm-kafka/README.md ---
@@ -86,7 +93,7 @@ The KafkaConfig class also has bunch of public variables
that controls your appl
public
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-145586698
@choang: the refactored code is pushed
@erikdw: I was referring to the internal Json structure used to store
offsets as shown by example below:
{
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-145443930
@hsun-cnnxty : can you please make your statement a bit more concrete
please? i.e., what other info is stored in a given kafka topic's consumer
state? (other than the o
Github user choang commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-145400703
I would make state more concrete, but I suppose your approach is fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on Git
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-145317404
Hi Chi,
Storm stores more than just offset/partition data in the "state", would it
be necessary to declare?
public interface StateStore {
publ
Github user choang commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-143655914
I recommend making your abstraction at the state store, so you would have:
```
public interface StateStore {
public void write(Partition p, long offset);
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-142364685
Erik,
Pushed the change to rename config from "storm" to "zookeeper" (also fixed
the typo). As of Kafka dependency, it is a good question. Actually I am no
Github user erikdw commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r40032442
--- Diff:
external/storm-kafka/src/jvm/storm/kafka/PartitionStateManagerFactory.java ---
@@ -0,0 +1,68 @@
+package storm.kafka;
+
+import backtype.s
Github user erikdw commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r40032400
--- Diff:
external/storm-kafka/src/jvm/storm/kafka/PartitionStateManagerFactory.java ---
@@ -0,0 +1,68 @@
+package storm.kafka;
+
+import backtype.s
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-142121623
@hsun-cnnxty : I don't see the reference links in the Description on the
PR's Conversation view? Maybe I'm looking in the wrong place? Maybe they
should be in comments i
Github user erikdw commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r40030760
--- Diff: external/storm-kafka/README.md ---
@@ -52,17 +52,24 @@ The optional ClientId is used as a part of the
zookeeper path where the spout's
There are
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-140633818
Erik,
I added 2 reference links in the description with more information on
"Kafka's consumer offset management api".
As of migration from existing o
Github user hsun-cnnxty commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r39595289
--- Diff:
external/storm-kafka/src/jvm/storm/kafka/PartitionStateManagerFactory.java ---
@@ -0,0 +1,68 @@
+package storm.kafka;
+
+import backt
Github user hsun-cnnxty commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r39595284
--- Diff: external/storm-kafka/src/jvm/storm/kafka/KafkaDataStore.java ---
@@ -0,0 +1,202 @@
+package storm.kafka;
+
+import com.google.common.c
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-140606694
Furthermore, can you please provide a reference to "Kafka's consumer offset
management api" in your description?
---
If your project is set up for it, you can reply to th
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-140556439
This seems like it might not be backwards compatible with the existing
kafka-spout; i.e., the offsets in ZK are presumably not going to be stored
exactly the same as they
Github user erikdw commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r39569887
--- Diff:
external/storm-kafka/src/jvm/storm/kafka/PartitionStateManagerFactory.java ---
@@ -0,0 +1,68 @@
+package storm.kafka;
+
+import backtype.s
Github user erikdw commented on a diff in the pull request:
https://github.com/apache/storm/pull/705#discussion_r39569807
--- Diff: external/storm-kafka/src/jvm/storm/kafka/KafkaDataStore.java ---
@@ -0,0 +1,202 @@
+package storm.kafka;
+
+import com.google.common.collec
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-139828793
Fixed coding styles suggested by rmkellogg.
---
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 pr
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-138791147
I assume you are referring to the json format of the spout "state" stored
in ZK? The "state" will be saved using the exact same json format in Kafka's
internal topic
Github user sweetest commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-13874
how will this change affect the format of spout's offset message in
zookeeper?
---
If your project is set up for it, you can reply to this email and have your
reply app
Github user hsun-cnnxty commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-138777397
Thanks for the advices. Will get them fixed as soon as I get some time.
---
If your project is set up for it, you can reply to this email and have your
reply appear o
Github user rmkellogg commented on the pull request:
https://github.com/apache/storm/pull/705#issuecomment-138665642
A few minor nits:
When providing javadoc on variables/methods, be sure to use the following
syntax:
/**
* Comment text
**/
private String v
GitHub user hsun-cnnxty opened a pull request:
https://github.com/apache/storm/pull/705
[STORM-1015] Allow Kafka offsets to be saved using Kafka's consumer offset
management api
Not sure when it will be reviewed. So I chose to implement it based on
master branch. Could be ported t
38 matches
Mail list logo
