[GitHub] storm pull request #2755: STORM-3082 Add support to handle absent topics

[aniketalhat]

Github user aniketalhat commented on a diff in the pull request:

https://github.com/apache/storm/pull/2755#discussion_r200802071
  
--- Diff: 
external/storm-kafka/src/jvm/org/apache/storm/kafka/DynamicPartitionConnections.java
 ---
@@ -57,6 +57,7 @@ public SimpleConsumer register(Partition partition) {
 
 public SimpleConsumer register(Broker host, String topic, int 
partition) {
 if (!_connections.containsKey(host)) {
+
--- End diff --

Not sure what happen here


---

[GitHub] storm issue #2755: STORM-3082 Add support to handle absent topics

[aniketalhat]

Github user aniketalhat commented on the issue:

https://github.com/apache/storm/pull/2755
  
@srdo I checked out my branch from master, I changed the target and we have 
conflicts, obviously. Do you recommend checking out from 1.x ?


---

Re: Requesting review for a couple of PRs

[Hugo Louro]

I’ll take a pass at reviewing them as well.
Thanks,
Hugo

> On Jul 6, 2018, at 1:41 PM, Jungtaek Lim  wrote:
> 
> Yeah I have been hoping that some other folks who are familiar with
> storm-kafka-client jump in and review in time, but unfortunately it didn't
> happen. Will try to review those PRs, hopefully within couple of days.
> 
> Btw, if there're missing PRs to resolve STORM-2953
>  I'd like to ask you to
> also add them here, so we can resolve the one which is effectively blocker
> for Storm 2.0.
> 
> Thanks,
> Jungtaek Lim (HeartSaVioR)
> 
> 2018년 7월 6일 (금) 오후 9:08, Stig Rohde Døssing 님이 작성:
> 
>> Hi devs,
>> 
>> There are a couple of PRs open for changes in storm-kafka-client that would
>> be nice to get reviewed. The PRs are
>> https://github.com/apache/storm/pull/2652
>> https://github.com/apache/storm/pull/2648 and
>> https://github.com/apache/storm/pull/2590. If someone could find a couple
>> of minutes to give them a look, I'd appreciate it.
>> 
>> Thanks.
>> 

[GitHub] storm issue #2739: [STORM-3125/3126/3127]: Refactoring components for Superv...

[zd-project]

Github user zd-project commented on the issue:

https://github.com/apache/storm/pull/2739
  
Should I also rebase it onto current master branch?


---

Re: Requesting review for a couple of PRs

[Jungtaek Lim]

Yeah I have been hoping that some other folks who are familiar with
storm-kafka-client jump in and review in time, but unfortunately it didn't
happen. Will try to review those PRs, hopefully within couple of days.

Btw, if there're missing PRs to resolve STORM-2953
 I'd like to ask you to
also add them here, so we can resolve the one which is effectively blocker
for Storm 2.0.

Thanks,
Jungtaek Lim (HeartSaVioR)

2018년 7월 6일 (금) 오후 9:08, Stig Rohde Døssing 님이 작성:

> Hi devs,
>
> There are a couple of PRs open for changes in storm-kafka-client that would
> be nice to get reviewed. The PRs are
> https://github.com/apache/storm/pull/2652
> https://github.com/apache/storm/pull/2648 and
> https://github.com/apache/storm/pull/2590. If someone could find a couple
> of minutes to give them a look, I'd appreciate it.
>
> Thanks.
>

[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2443
  
@HeartSaVioR Are you still planning on getting this in? I could probably 
put some time in to trying this out if no one else is willing to test/use 
storm-sql, so we can get a +1.


---

[GitHub] storm pull request #2726: STORM-3090 - Fix bug when different topics use the...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2726


---

[GitHub] storm issue #2726: STORM-3090 - Fix bug when different topics use the same o...

[choojoyq]

Github user choojoyq commented on the issue:

https://github.com/apache/storm/pull/2726
  
@srdo i think so, thanks for the review


---

[GitHub] storm issue #2726: STORM-3090 - Fix bug when different topics use the same o...

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2726
  
Is this ready to merge @choojoyq?


---

[GitHub] storm pull request #2726: STORM-3090 - Fix bug when different topics use the...

[srdo]

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2726#discussion_r200657323
  
--- Diff: 
external/storm-kafka/src/test/org/apache/storm/kafka/ZkCoordinatorTest.java ---
@@ -140,6 +140,42 @@ public void testPartitionManagerRecreate() throws 
Exception {
 }
 }
 
+@Test
+public void testPartitionManagerRecreateMultipleTopics() throws 
Exception {
+List coordinatorList = buildCoordinators(1);
+
+
when(reader.getBrokerInfo()).thenReturn(TestUtils.buildPartitionInfoList(
+TestUtils.buildPartitionInfo("topic1", 1, 9092),
+TestUtils.buildPartitionInfo("topic2", 1, 9092)));
+
+List partitionManagersBeforeRefresh = 
coordinatorList.get(0).getMyManagedPartitions();
+assertEquals(2, partitionManagersBeforeRefresh.size());
+for (PartitionManager partitionManager : 
partitionManagersBeforeRefresh) {
+partitionManager._emittedToOffset = 100L;
+partitionManager._committedTo = 100L;
+}
+
+waitForRefresh();
+
+
when(reader.getBrokerInfo()).thenReturn(TestUtils.buildPartitionInfoList(
+TestUtils.buildPartitionInfo("topic1", 1, 9093),
+TestUtils.buildPartitionInfo("topic3", 1, 9093)));
--- End diff --

Thanks, it makes sense. Since there's only one task, topic 2 can't be 
removed though. It's not really important, the test makes sense regardless.


---

[GitHub] storm pull request #2726: STORM-3090 - Fix bug when different topics use the...

[choojoyq]

Github user choojoyq commented on a diff in the pull request:

https://github.com/apache/storm/pull/2726#discussion_r200655178
  
--- Diff: 
external/storm-kafka/src/test/org/apache/storm/kafka/ZkCoordinatorTest.java ---
@@ -140,6 +140,42 @@ public void testPartitionManagerRecreate() throws 
Exception {
 }
 }
 
+@Test
+public void testPartitionManagerRecreateMultipleTopics() throws 
Exception {
+List coordinatorList = buildCoordinators(1);
+
+
when(reader.getBrokerInfo()).thenReturn(TestUtils.buildPartitionInfoList(
+TestUtils.buildPartitionInfo("topic1", 1, 9092),
+TestUtils.buildPartitionInfo("topic2", 1, 9092)));
+
+List partitionManagersBeforeRefresh = 
coordinatorList.get(0).getMyManagedPartitions();
+assertEquals(2, partitionManagersBeforeRefresh.size());
+for (PartitionManager partitionManager : 
partitionManagersBeforeRefresh) {
+partitionManager._emittedToOffset = 100L;
+partitionManager._committedTo = 100L;
+}
+
+waitForRefresh();
+
+
when(reader.getBrokerInfo()).thenReturn(TestUtils.buildPartitionInfoList(
+TestUtils.buildPartitionInfo("topic1", 1, 9093),
+TestUtils.buildPartitionInfo("topic3", 1, 9093)));
--- End diff --

The idea of the test that before rebalancing some ``Task`` was responsible 
for 2 partitions from topics 1 and 2 from broker 9092, but after rebalancing of 
Kafka brokers it became responsible for the same partition for topic 1 and 
another partition from topic 3 which are hosted now on broker 9093. So ``Task`` 
should preserve partition information about topic1 as it was managed by this 
task before and it knows how many messages was already emitted, committed, etc. 
While partition from topic3 is a brand new and so it should be created from 
scratch without reusing of any information. Before the fix information for 
topic3 would be reused from topic1 as lookup of previously managed partitions 
was performed only on partition number ignoring topic.


---

[GitHub] storm pull request #2726: STORM-3090 - Fix bug when different topics use the...

[srdo]

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2726#discussion_r200653436
  
--- Diff: 
external/storm-kafka/src/jvm/org/apache/storm/kafka/ZkCoordinator.java ---
@@ -107,14 +110,14 @@ public void refresh() {
 _stormConf,
 _spoutConfig,
 id,
-deletedManagers.get(id.partition));
+deletedManagers.get(new 
TopicAndPartition(id.topic, id.partition)));
--- End diff --

Good point, never mind then.


---

[GitHub] storm pull request #2726: STORM-3090 - Fix bug when different topics use the...

[choojoyq]

Github user choojoyq commented on a diff in the pull request:

https://github.com/apache/storm/pull/2726#discussion_r200651516
  
--- Diff: 
external/storm-kafka/src/jvm/org/apache/storm/kafka/ZkCoordinator.java ---
@@ -107,14 +110,14 @@ public void refresh() {
 _stormConf,
 _spoutConfig,
 id,
-deletedManagers.get(id.partition));
+deletedManagers.get(new 
TopicAndPartition(id.topic, id.partition)));
--- End diff --

I believe that this lookup exists for preserving information about 
partitions from other brokers in case of rebalancing. If for example one of 
brokers fail and its partitions are reassigned to other brokers this lookup 
should find information about this partition and reuse it during recreation of 
the same partition but on other host. ``equals()`` and ``hashCode()`` in 
Partition are based on topic, partition and host while this lookup in based 
only on topic and partition.


---

Requesting review for a couple of PRs

[Stig Rohde Døssing]

Hi devs,

There are a couple of PRs open for changes in storm-kafka-client that would
be nice to get reviewed. The PRs are
https://github.com/apache/storm/pull/2652
https://github.com/apache/storm/pull/2648 and
https://github.com/apache/storm/pull/2590. If someone could find a couple
of minutes to give them a look, I'd appreciate it.

Thanks.

[GitHub] storm issue #2755: STORM-3082 Add support to handle absent topics

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2755
  
I guess this should target 1.0.x-branch, going by the listed commits.


---

[GitHub] storm issue #2755: STORM-3082 Add support to handle absent topics

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2755
  
@aniketalhat I think you are targeting the wrong branch. You should target 
the branch you started working on (e.g. 1.x-branch).

We can review 1.x first, but we'll also need a PR against master at some 
point before we can merge this.


---

[GitHub] storm pull request #2755: [WIP] STORM-3082 Add support to handle absent topi...

[aniketalhat]

GitHub user aniketalhat opened a pull request:

https://github.com/apache/storm/pull/2755

[WIP] STORM-3082 Add support to handle absent topics



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/aniketalhat/storm 
feature/STORM-3082-Add-support-to-handle-absent-topics

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/2755.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2755


commit a06f52105d12723ade3de0741c6cf90d0b1bcef0
Author: Jungtaek Lim 
Date:   2016-11-25T01:18:55Z

STORM-2216: CHANGELOG

commit c9b80da918510e16ce239a4e8bfbb0b8c22f9358
Author: Kyle Nusbaum 
Date:   2016-11-28T19:09:17Z

STORM- Repeated NPEs thrown in nimbus if rebalance fails

commit 0fd5ef1efa24de9e2f90b238c98a91967b5bd43d
Author: itiel 
Date:   2016-12-03T16:45:04Z

fix actiavte and deactive heartbeat problem

commit 639bfb5a97f78b87f023ffa873d4fdc43a3b67ab
Author: Jungtaek Lim 
Date:   2016-12-06T23:14:33Z

Merge branch 'STORM-2234-1.0.x-merge' into 1.0.x-branch

commit d56e76c16cd214e4516c83358fbbe07d57ae346f
Author: Jungtaek Lim 
Date:   2016-12-06T23:14:46Z

STORM-2234: CHANGELOG

commit 04b86a272776b1c8de8de81847cc1f9dbb55e5a7
Author: Kyle Nusbaum 
Date:   2016-12-02T19:46:21Z

Updating Trident RAS Documentation.

commit 9792679bb633c08425692f788398c01099589ed0
Author: Jungtaek Lim 
Date:   2016-12-06T23:39:26Z

Merge branch 'PR-1812-1.0.x-merge' into 1.0.x-branch

commit 73523c99d159ad42ae6f630e7801070e8d6564c3
Author: Hugo Louro 
Date:   2016-12-17T08:57:13Z

Fix typo in flux.md

commit 9365456af2a26c4a240ff304c7fb331e423f1868
Author: Jungtaek Lim 
Date:   2016-12-18T11:01:41Z

STORM-2251 Integration test refers specific version of Storm which should 
be project version

* fix integration-test's pom.xml to have root pom as parent, and remove 
version and storm version

commit 49957f500a17931cf07c0b7352bebadb38693ed5
Author: Jungtaek Lim 
Date:   2016-12-22T15:04:44Z

Merge branch 'STORM-2251-1.0.x-merge' into 1.0.x-branch

commit fabd37bdb27974e77b1dcd4c639f3f60a3794591
Author: Jungtaek Lim 
Date:   2016-12-22T15:04:56Z

STORM-2251: CHANGELOG

commit 61935a7debaca02710e4cb05301098b0a17f9de7
Author: Kyle Nusbaum 
Date:   2016-12-22T18:34:30Z

Merge branch '1.0.x-branch' of https://github.com/knusbaum/incubator-storm 
into HEAD

commit 07410fe6901ff1c38b35fc04d7c115413b5c3335
Author: Kyle Nusbaum 
Date:   2016-12-22T18:37:00Z

Updating CHANGELOG

commit 6a922002e96eadfcab635cacbeceed8800f53bd3
Author: Wissman, Matthew 
Date:   2016-12-29T15:29:39Z

STORM-2095 remove any remaining files when deleting blobstore directory

commit c38cac8822a6d78ba659e8b06b32d076c1858101
Author: Jungtaek Lim 
Date:   2017-01-03T14:13:13Z

Merge branch '1.0.x-branch' of https://github.com/mwissman/storm into 
STORM-2095-1.0.x-merge

commit 0991fad22cd84edc16f33e8c24dc52dc81328a9f
Author: Jungtaek Lim 
Date:   2017-01-03T14:13:41Z

STORM-2095: CHANGELOG

commit 9027c00bcce7214308fdaf5e6aa6b6a818cbe6f1
Author: Jungtaek Lim 
Date:   2017-01-05T04:08:34Z

STORM-2276 Remove twitter4j usages due to license issue (JSON.org is 
catalog X)

commit b266aa180b4893d6fc883dd1402dc40ab2360cf7
Author: Jungtaek Lim 
Date:   2017-01-06T08:55:17Z

Merge branch 'STORM-2276-1.0.x' into 1.0.x-branch

commit ce0b155d5cf5810c0bca637db5361c094c40657b
Author: Jungtaek Lim 
Date:   2017-01-06T08:55:33Z

STORM-2276: CHANGELOG

commit 451ef9358f03d153b142fb2187ed6e9c562c8faf
Author: Jungtaek Lim 
Date:   2017-01-04T16:24:25Z

STORM-2264 OpaqueTridentKafkaSpout failing after STORM-2216

* use JSONValue.parse() instead of JSONValue.parseWithException() in 
TransactionState
  * this just rolls back to previous, doesn't provide better approach

commit 6ab374b8a25de26f9edbeebf022766aa1802a82d
Author: Jungtaek Lim 
Date:   2017-01-08T23:51:03Z

Merge branch 'STORM-2264-1.0.x-merge' into 1.0.x-branch

commit 3b0837ee408661a67aefded21662b50e7ec1d9a5
Author: Jungtaek Lim 
Date:   2017-01-08T23:52:39Z

STORM-2264: CHANGELOG

commit 5f3fe962973822ebcdc5829b652ccffbc4185492
Author: Raghav Kumar Gautam 
Date:   2017-01-09T19:17:05Z

STORM-2279: Unable to open bolt page of storm ui

commit ac3b1b9283de49f15cbcc69dff49b5d732dd36c1
Author: Jungtaek Lim 
Date:   2017-01-10T00:53:25Z

Merge branch '1.0.x-branch' of https://github.com/raghavgautam/storm into 
STORM-2279-1.0.x-merge

commit 4b44b40ea12ca3f2cda88ee471fe666029fe40e5
Author: Jungtaek Lim 
Date:   2017-01-10T00:54:38Z

STORM-2279: CHANGELOG

commit e4a83d8fef6f397e15cee659b2d805b229fcf006
Author: Arun Mahadevan 
Date:   2017-01-11T06:48:31Z

[STORM-2283] Fix multi threading issues in DefaultStateSerializer

commit 5f60bc4db5e47e927b5093bbe2721989a98182ed
Author: Jungtaek