[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii closed the pull request at: https://github.com/apache/storm/pull/1470 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1470 @arunmahadevan yes, check #1811 please --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1811: STORM-1886 Extend KeyValueState iface with delete
GitHub user kosii opened a pull request: https://github.com/apache/storm/pull/1811 STORM-1886 Extend KeyValueState iface with delete The same as #1470, just against the [1.x-branch](https://github.com/apache/storm/tree/1.x-branch) This pull request incorporates #1606 and #1160, because my patch depends on #1606, and #1606 depends on #1160. Compared to #1470, I had to add a new commit f115162 , because [1.x-branch](https://github.com/apache/storm/tree/1.x-branch) uses Java 1.7. You can merge this pull request into a Git repository by running: $ git pull https://github.com/s4mDev/storm keyvaluestate-with-delete-1.x Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1811.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 #1811 commit f6f35dd98d2492a38aa4d61da7f6caee4ec2f31a Author: Alessandro Bellina <abell...@yahoo-inc.com> Date: 2016-02-27T04:42:59Z STORM-1228: port fields_test to java commit e5ae496e78aaf7a20a05bcff9139e2a6fd1ad551 Author: Alessandro Bellina <abell...@yahoo-inc.com> Date: 2016-02-29T23:55:29Z STORM-1228: code review comments commit cdf80153009db83527bfbf0e5f3ab61126c5475e Author: Alessandro Bellina <abell...@yahoo-inc.com> Date: 2016-03-02T06:32:37Z STORM-1228: instantiate fields in each test commit 6d3a90d53ea425881b8b3c20a52932c7977bbccc Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-08-04T15:43:02Z STORM-2020: Stop using sun internal classes. commit 3d92358ef5a4e1be5dd86d32c960279d5f824f91 Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-08-04T16:24:08Z Addressed some review comments commit 44f0beb18fe4b4c304fa73339df02a6abee99445 Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-08-04T16:29:24Z STORM-2022: update Fields test to match new behavior commit d55ddffbe6c5bc2c7f022b530e5acfc0bb50c07f Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-06-02T12:32:06Z STORM-1886 Extend KeyValueState iface with delete The patch also provides implementation for delete in RedisKeyValueState and InMemoryKeyValueState. commit 2eb308a6d36463b758d1f571cfeb6c9098176d40 Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-06-24T15:51:32Z Use a Map with tombstones to represent deletions commit 69068706fd43406a1a8eba63ebbc66932c174cef Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-11-28T16:24:16Z STORM-1886 Address last comments * make tombstone static * hide the use of Optional from the users * fix race condition * make encode and decode methods static commit f115162f7ae65257d574a729426c845486a057ed Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-12-02T12:39:06Z STORM-1886 Make patch compatible with Java 1.7 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1470 @arunmahadevan actually my patch depends on #2020, which isn't yet in 1.x-branch. how should we proceed? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1809: STORM-1886 Extend KeyValueState iface with delete
Github user kosii closed the pull request at: https://github.com/apache/storm/pull/1809 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1809: STORM-1886 Extend KeyValueState iface with delete
GitHub user kosii opened a pull request: https://github.com/apache/storm/pull/1809 STORM-1886 Extend KeyValueState iface with delete The same as #1470, just against the 1.x-branch You can merge this pull request into a Git repository by running: $ git pull https://github.com/s4mDev/storm keyvaluestate-with-delete Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1809.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 #1809 commit 3a16070385e699af37d996fb2401260ec671a391 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T01:39:04Z add STORM-2101 to CHANGELOG commit edd0c73d71dceeeb0e2d645a8d96da8fc1c13aa3 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T01:43:33Z Merge branch 'STORM-2110' of https://github.com/revans2/incubator-storm into STORM-2110 commit 1bdc922e288dac031cdb9bc628e3dd660385dd03 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T01:44:02Z add STORM-2110 to CHANGELOG commit bd75c9352c45d7e7c16b46052c0a3c4e9b2513d2 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T01:48:23Z Merge branch 'master' of https://github.com/aichow/storm into STORM-2120 commit 5e5e83cabe3b66161d4774f5f5698fcad8012d0e Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T01:48:43Z add STORM-2120 to CHANGELOG commit 1fed08f3ee56de4bc98adc8991c7b2c76f9bf4ec Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T01:56:21Z Merge branch 'STORM-1664' of https://github.com/srdo/storm into STORM-1664 commit eb02edeaa8e7e6daf0012c68650d35b5fd00baa6 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T01:56:49Z add STORM-1664 to CHANGELOG commit 7140a1534c32b1ead35c4c35a251a543f2e0b488 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T02:02:58Z Merge branch 'STORM-2078_enable_pagination_in_worker_tables' of https://github.com/abellina/storm into STORM-2078 commit d2ca97f3e96e382f33978893e2fad9a887933b76 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-23T02:13:29Z add STORM-2078 to CHANGELOG commit a7445d3f714e4eee913997689f87ad53b5483ac5 Author: Arun Mahadevan <ar...@apache.org> Date: 2016-09-23T05:57:45Z [STORM-2118] address review comments commit 6cd9661bde2ce4c93c5628aae2565e167b89aeb3 Author: Kyle Nusbaum <kylejnusb...@gmail.com> Date: 2016-09-23T06:48:33Z Fix ups. commit 6a8be9f8c19bacb646c119c162c6cdba69cc66ff Author: Kyle Nusbaum <kylejnusb...@gmail.com> Date: 2016-09-23T07:03:46Z Fix ups. commit 523317b06abe87d7e745885c32c8a9c5dfbbeb0d Author: Kyle Nusbaum <kylejnusb...@gmail.com> Date: 2016-09-23T07:29:59Z Cleaning up tests. commit 410ef3a816fc6a961771abfbcac2d83512192040 Author: Kyle Nusbaum <kylejnusb...@gmail.com> Date: 2016-09-23T07:31:54Z Cleaning up. commit 158228b7ad434287e7d1ec92aca9c5c08f144869 Author: Kyle Nusbaum <kylejnusb...@gmail.com> Date: 2016-09-23T16:03:32Z Kick travis commit e55d9b7d47a8ea62174be106af0f9ab1cea402eb Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-09-23T16:26:34Z STORM-2122: Cache dependency data, and serialize reading of the data. commit 6a657f16301306b5b0d8d4ab91f916c6e7757ffe Author: Kyle Nusbaum <kylejnusb...@gmail.com> Date: 2016-09-23T16:28:39Z Fixing travis, adding license to new code. commit e5a8841d3138eb5b7ce7863b0b2a1c8452bb8b5c Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-09-21T14:41:15Z STORM-2109: Treat Supervisor CPU/MEMORY Configs as Numbers commit bf91e6d27912d02912d7220d2abeeaf05b8d5f0a Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-09-23T17:07:25Z Addressed review comments commit 7bb3b25473b7271512715f0599f27c334ef61332 Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-09-23T17:28:03Z Merge branch 'STORM-2117' of https://github.com/HeartSaVioR/storm into STORM-2117 STROM-2117: Supervisor V2 with local mode extracts resources directory to the wrong directory commit d8115e7bdfeb08a9629a6e8525b90de72c35cbf7 Author: Robert (Bobby) Evans <ev...@yahoo-inc.com> Date: 2016-09-23T17:28:44Z Added STORM-2117 to Changelog commit 42722c5f1707d8179adf2903480744327b326954 Author: kamleshbhatt <kbhatt2...@gmail.com> Date: 2016-09-23T17:48:51Z correction as per review comments commit dcd4de631cc3b4d5752da8e0246c658c9b0dca24 Author: kamleshbhatt <kbhatt2...@gmail.com> Date: 2016-09-23T17:52:08Z corrections as per review comments commit 506b77ed2428d7554000d2bcdbb036c55c068825 Author: Jungtaek Lim <kabh...@gmail.com> Date: 2016-09-25T11:17:34Z Merge branch 'STORM-2105_cluster_supervisor_total_and_avail_resources' of https://github.com/abell
[GitHub] storm issue #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1470 Yes, I'll look into that! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1470 @arunmahadevan I'm very sorry for disappearing for such a long time. I addressed all the comments, I also fixed the race condition in my last commit --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1470 @HeartSaVioR I addressed most of the comments in my local repo, but not pushed yet. If I have some time this week (probably Friday) I'll clean it up, and commit. Sorry for the delay --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1470#discussion_r69322053
--- Diff:
external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java
---
@@ -161,7 +176,11 @@ public void prepareCommit(long txid) {
commands = jedisContainer.getInstance();
if (commands.exists(prepareNamespace)) {
LOG.debug("Prepared txn already exists, will merge", txid);
-pendingPrepare.putAll(pendingCommit);
+for (Map.Entry<String, String> e:
pendingCommit.entrySet()) {
+if (!pendingPrepare.containsKey(e.getKey())) {
+pendingPrepare.put(e.getKey(), e.getValue());
+}
+}
}
if (!pendingPrepare.isEmpty()) {
commands.hmset(prepareNamespace, pendingPrepare);
--- End diff --
I'd fix it in a new JIRA, that way it might be released sooner
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1470#discussion_r69321607
--- Diff:
external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java
---
@@ -60,22 +64,23 @@ public RedisKeyValueState(String namespace) {
}
public RedisKeyValueState(String namespace, JedisPoolConfig
poolConfig) {
-this(namespace, poolConfig, new DefaultStateSerializer(), new
DefaultStateSerializer());
+this(namespace, poolConfig, new DefaultStateSerializer(), new
DefaultStateSerializer<Optional>());
}
-public RedisKeyValueState(String namespace, JedisPoolConfig
poolConfig, Serializer keySerializer, Serializer valueSerializer) {
+public RedisKeyValueState(String namespace, JedisPoolConfig
poolConfig, Serializer keySerializer, Serializer<Optional>
valueSerializer) {
this(namespace, JedisCommandsContainerBuilder.build(poolConfig),
keySerializer, valueSerializer);
}
public RedisKeyValueState(String namespace,
JedisCommandsInstanceContainer jedisContainer,
- Serializer keySerializer, Serializer
valueSerializer) {
+ Serializer keySerializer,
Serializer<Optional> valueSerializer) {
base64Encoder = new BASE64Encoder();
base64Decoder = new BASE64Decoder();
this.namespace = namespace;
this.prepareNamespace = namespace + "$prepare";
this.txidNamespace = namespace + "$txid";
this.keySerializer = keySerializer;
this.valueSerializer = valueSerializer;
+this.tombstone =
encode(valueSerializer.serialize(Optional.absent()));
--- End diff --
I'm not sure, but I don't think so, as the `internalValueSerializer` would
depend on the `V` type parameter
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1470#discussion_r69321335
--- Diff:
external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java
---
@@ -60,22 +64,23 @@ public RedisKeyValueState(String namespace) {
}
public RedisKeyValueState(String namespace, JedisPoolConfig
poolConfig) {
-this(namespace, poolConfig, new DefaultStateSerializer(), new
DefaultStateSerializer());
+this(namespace, poolConfig, new DefaultStateSerializer(), new
DefaultStateSerializer<Optional>());
}
-public RedisKeyValueState(String namespace, JedisPoolConfig
poolConfig, Serializer keySerializer, Serializer valueSerializer) {
+public RedisKeyValueState(String namespace, JedisPoolConfig
poolConfig, Serializer keySerializer, Serializer<Optional>
valueSerializer) {
--- End diff --
by having a `private final Serializer<Optional> internalValueSerializer`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1470 A short list of modifications - I went for the `Optional` class. Using your idea largely simplified the code. - delete now returns the current value - I didn't use a different set to store `pendingPrepare`, the deleted keys are represented with tombstones in the redis hash --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1470#discussion_r68263894
--- Diff:
external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java
---
@@ -203,12 +237,14 @@ public void commit() {
JedisCommands commands = null;
try {
commands = jedisContainer.getInstance();
-if (!pendingPrepare.isEmpty()) {
+if (!pendingPrepare.isEmpty() ||
!pendingDeletePrepare.isEmpty()) {
commands.hmset(namespace, pendingPrepare);
--- End diff --
Yes, you're right!
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1470#discussion_r68263881
--- Diff:
external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java
---
@@ -184,15 +216,17 @@ public void commit(long txid) {
JedisCommands commands = null;
try {
commands = jedisContainer.getInstance();
-if (!pendingCommit.isEmpty()) {
+if (!pendingCommit.isEmpty() ||
!pendingDeleteCommit.isEmpty()) {
commands.hmset(namespace, pendingCommit);
--- End diff --
Yes, you're right!
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request: https://github.com/apache/storm/pull/1470#discussion_r68261133 --- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java --- @@ -77,7 +81,8 @@ public RedisKeyValueState(String namespace, JedisCommandsInstanceContainer jedis this.keySerializer = keySerializer; this.valueSerializer = valueSerializer; this.jedisContainer = jedisContainer; -this.pendingPrepare = new ConcurrentHashMap<>(); --- End diff -- Using guava's `Optional` seems like a good idea to you? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request: https://github.com/apache/storm/pull/1470#discussion_r68260812 --- Diff: storm-core/src/jvm/org/apache/storm/state/KeyValueState.java --- @@ -45,4 +45,11 @@ * @return the value or defaultValue if no mapping is found */ V get(K key, V defaultValue); + +/** + * Deletes the value mapped to the key, if there is any + * + * @param key the key + */ +void delete(K key); --- End diff -- It was my initial idea, but in case of the Redis implementation it needs and extra database request , as the key might not currently in one of the `pending*` structures. Does the overhead of the extra query looks negligible to you? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1470#discussion_r68260007
--- Diff:
external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java
---
@@ -163,15 +190,20 @@ public void prepareCommit(long txid) {
LOG.debug("Prepared txn already exists, will merge", txid);
pendingPrepare.putAll(pendingCommit);
}
-if (!pendingPrepare.isEmpty()) {
+if (!pendingPrepare.isEmpty() ||
!pendingDeletePrepare.isEmpty()) {
commands.hmset(prepareNamespace, pendingPrepare);
+commands.hdel(prepareNamespace,
pendingDeletePrepare.toArray(new String[pendingDeletePrepare.size()]));
--- End diff --
Yes, you are right, I forget easily that the
`pendingPrepare`/`pendingCommit` doesn't actually contain the snapshot, just a
delta
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1470: STORM-1886 Extend KeyValueState iface with delete
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1470 Thanks @arunmahadevan for the thorough review, I'm gonna address each comments asap --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1453: STORM-1873 Implement alternative behaviour for late tuple...
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1453 @arunmahadevan @satishd nit is fixed and commits are squashed. thanks for the review! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1470: STORM-1886 Extend KeyValueState iface with delete
GitHub user kosii opened a pull request: https://github.com/apache/storm/pull/1470 STORM-1886 Extend KeyValueState iface with delete The patch also provides implementation for delete in RedisKeyValueState and InMemoryKeyValueState. You can merge this pull request into a Git repository by running: $ git pull https://github.com/s4mDev/storm keyvaluestate-with-delete Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1470.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 #1470 commit 03e7c7276a8b5bc0e913ad4d28e7100101d6c029 Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-06-02T12:32:06Z STORM-1886 Extend KeyValueState iface with delete The patch also provides implementation for delete in RedisKeyValueState and InMemoryKeyValueState. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1464: STORM-1884: Prioritize pendingPrepare over pending...
GitHub user kosii opened a pull request: https://github.com/apache/storm/pull/1464 STORM-1884: Prioritize pendingPrepare over pendingCommit You can merge this pull request into a Git repository by running: $ git pull https://github.com/s4mDev/storm fix_storm_1884 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1464.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 #1464 commit d5dbbe0e3207508f28d4dfae263ce930743a049d Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-06-06T15:04:27Z Fix STORM-1884 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1453: STORM-1873 Implement alternative behaviour for lat...
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1453#discussion_r65866008
--- Diff:
storm-core/src/jvm/org/apache/storm/topology/WindowedBoltExecutor.java ---
@@ -188,6 +189,10 @@ private void validate(Map stormConf, Count
windowLengthCount, Duration windowLen
}
waterMarkEventGenerator = new
WaterMarkEventGenerator<>(manager, watermarkInterval,
maxLagMs, getComponentStreams(context));
+} else {
+if
(stormConf.containsKey(Config.TOPOLOGY_BOLTS_TUPLE_TIMESTAMP_FIELD_NAME)) {
--- End diff --
Yes, you are right. I'm gonna add more unit tests
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #939: [STORM-1175] State store for windowing operations
Github user kosii commented on the issue: https://github.com/apache/storm/pull/939 @arunmahadevan yes, I'm on it --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #:
Github user kosii commented on the pull request: https://github.com/apache/storm/commit/4623d8f87f044a9a8737a55bd32f955485900ce1#commitcomment-17749543 In external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java: In external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java on line 162: Keys in `pendingPrepare` should not have priority over the keys in `pendingCommit` as in [line 126](https://github.com/apache/storm/commit/4623d8f87f044a9a8737a55bd32f955485900ce1#diff-2e47d8842f277cb12f6bddbe2b7f94f5R126)? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #:
Github user kosii commented on the pull request: https://github.com/apache/storm/commit/4623d8f87f044a9a8737a55bd32f955485900ce1#commitcomment-17749399 In external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java: In external/storm-redis/src/main/java/org/apache/storm/redis/state/RedisKeyValueState.java on line 162: Keys in pendingPrepare should not have priority over the keys in pendingCommit? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1453: STORM-1873 Implement alternative behaviour for lat...
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1453#discussion_r65580954
--- Diff:
storm-core/src/jvm/org/apache/storm/topology/WindowedBoltExecutor.java ---
@@ -284,6 +298,10 @@ public void cleanup() {
@Override
public void declareOutputFields(OutputFieldsDeclarer declarer) {
+String lateTupleStream = (String)
getComponentConfiguration().get(Config.TOPOLOGY_BOLTS_LATE_TUPLE_STREAM);
+if (lateTupleStream != null) {
+declarer.declareStream(lateTupleStream, new
Fields("late_tuple"));
--- End diff --
should it be public, so that users could refer to it by the variable name?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1453: STORM-1873 Implement alternative behaviour for late tuple...
Github user kosii commented on the issue: https://github.com/apache/storm/pull/1453 I updated the docs, and I'm going to work on the unit tests while the patch is being reviewed --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1453: STORM-1873 Implement alternative behaviour for lat...
Github user kosii commented on a diff in the pull request: https://github.com/apache/storm/pull/1453#discussion_r65557613 --- Diff: storm-core/src/jvm/org/apache/storm/Config.java --- @@ -1872,6 +1872,13 @@ @isString public static final String TOPOLOGY_BOLTS_TUPLE_TIMESTAMP_FIELD_NAME = "topology.bolts.tuple.timestamp.field.name"; +/** + * Bolt-specific configuration for windowed bolts to specify whether late tuples should be emitted on a stream + * called _late, instead of being logged with INFO level. + */ +@isBoolean +public static final String TOPOLOGY_BOLTS_EMIT_LATE_TUPLE = "topology.bolts.emit.late.tuple"; --- End diff -- Cool, I'm going to update this pull request with the first version --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #1453: STORM-1873 Implement alternative behaviour for lat...
Github user kosii commented on a diff in the pull request:
https://github.com/apache/storm/pull/1453#discussion_r65557653
--- Diff:
storm-core/src/jvm/org/apache/storm/topology/WindowedBoltExecutor.java ---
@@ -268,8 +277,12 @@ public void execute(Tuple input) {
if
(waterMarkEventGenerator.track(input.getSourceGlobalStreamId(), ts)) {
windowManager.add(input, ts);
} else {
+if (emitLateTuples()) {
+windowedOutputCollector.emit(LATE_TUPLE_STREAM, input,
new Values(input));
+} else {
+LOG.info("Received a late tuple {} with ts {}. This
will not processed.", input, ts);
--- End diff --
fixed
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1453: STORM-1873 Implement alternative behaviour for lat...
Github user kosii commented on a diff in the pull request: https://github.com/apache/storm/pull/1453#discussion_r65527974 --- Diff: storm-core/src/jvm/org/apache/storm/Config.java --- @@ -1872,6 +1872,13 @@ @isString public static final String TOPOLOGY_BOLTS_TUPLE_TIMESTAMP_FIELD_NAME = "topology.bolts.tuple.timestamp.field.name"; +/** + * Bolt-specific configuration for windowed bolts to specify whether late tuples should be emitted on a stream + * called _late, instead of being logged with INFO level. + */ +@isBoolean +public static final String TOPOLOGY_BOLTS_EMIT_LATE_TUPLE = "topology.bolts.emit.late.tuple"; --- End diff -- I realized that it was hard to achieve a consistent behavior in the earlier implementation, because at the moment of calling `declareOutputFields`, we only have access to the component specific parameters, but if the user puts the key in the global config but forgets to specify at component level, then the stream won't be declared, but in the `execute` method it will still going to try to emit messages to the non-existing stream. On the other hand in the `initWindowManager` method, I can check whether the stream exist, and raise an exception if not. Does it seem okay to you? Because in this case I'd also prefer the per component based approach. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1873 Implement alternative behaviour for late tupl...
GitHub user kosii opened a pull request: https://github.com/apache/storm/pull/1453 STORM-1873 Implement alternative behaviour for late tuples You can merge this pull request into a Git repository by running: $ git pull https://github.com/kosii/storm late-tuple-poc-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1453.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 #1453 commit 7fedf89e75ba414be3894de7dd316394eb91baa5 Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-05-31T12:29:46Z Implement alternative behaviour for late tuples --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-1859: Ack late tuples in windowed mode
Github user kosii commented on the pull request: https://github.com/apache/storm/pull/1437#issuecomment-220916990 @arunmahadevan cool, done --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: Ack late tuples in windowed mode
GitHub user kosii opened a pull request: https://github.com/apache/storm/pull/1437 Ack late tuples in windowed mode The current implementation simply ignores late tuples without acking them, which causes timeouts and replays after TOPOLOGY_MESSAGE_TIMEOUT_SECS. A tuple which was late at a some time is going to be late at any moment in the future, so there is no point of replaying it, because the lingering late tuples will just block the topology (especially if TOPOLOGY_MAX_SPOUT_PENDING is set). You can merge this pull request into a Git repository by running: $ git pull https://github.com/kosii/storm master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1437.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 #1437 commit 7b24dfa6523322b84f496d9533b87cb8bedb2461 Author: Balazs Kossovics <balazs.kossov...@s4m.io> Date: 2016-05-20T15:36:42Z Ack late tuples in windowed mode The current implementation simply ignores late tuples without acking them, which causes timeouts and replays after TOPOLOGY_MESSAGE_TIMEOUT_SECS. A tuple which was late at a some time is going to be late at any moment in the future, so there is no point of replaying it, because the lingering late tuples will just block the topology (especially if TOPOLOGY_MAX_SPOUT_PENDING is set). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
