You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by ikashperskyi <gi...@git.apache.org> on 2016/12/10 13:55:41 UTC
[GitHub] storm pull request #1822: Storm 2121
GitHub user ikashperskyi opened a pull request:
https://github.com/apache/storm/pull/1822
Storm 2121
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ikashperskyi/storm STORM-2121
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1822.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 #1822
----
commit a5bd246342316a091d7871f4fa77ffbeb8bc0a1e
Author: ikashperskyi <ze...@gmail.com>
Date: 2016-12-10T13:30:41Z
Inlining deserialization and adding deserializeString as static import
commit 941d323718cc2e6747eb8670b469213a1d04d0dd
Author: ikashperskyi <ze...@gmail.com>
Date: 2016-12-10T13:36:18Z
STORM-2121: Overriding StringKeyValueScheme.getOutputFields to contain both key and value
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by ikashperskyi <gi...@git.apache.org>.
Github user ikashperskyi commented on the issue:
https://github.com/apache/storm/pull/1822
@harshach @HeartSaVioR I feel there's more to it than that. My assumption now is only 1 field is declared because we're falling back to only deserialising the message given a null key. If we were to declare both fields the actual deserialisation should look like:
return new Values(key == null ? StringUtils.EMPTY : deserializeString(key), deserializeString(value));
I'll adjust my PR if you agree that this is the way to go.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1822
Sorry I'm revoking my +1. It's not a bug though I don't know why we decided to use 1 field with map. And we're breaking backward compatibility with widely-used module, so need to be thoughtful about this.
In fact this issue came from STORM-2123. Instead of changing existing attribute, we could add ByteKeyValueScheme to have same attribute.
@asmaier @ikashperskyi @vesense What do you think?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1822
+1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1822
@ikashperskyi I would like to also address this to 1.x so keeping backward compatibility is ideal for now.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1822
@ikashperskyi @HeartSaVioR StringKeyValueScheme should be emitting both key and value. Its a bug that we are not declaring the key field. For emiting only value users can config StringScheme.
This fix looks good to me , +1 on merging.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/1822
+1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by ikashperskyi <gi...@git.apache.org>.
Github user ikashperskyi commented on the issue:
https://github.com/apache/storm/pull/1822
@wurstmeister could you advise if this is a bug or why did we go with only one field if it's not?
@HeartSaVioR this would go into the next major release so I would worry about backward compatibility too much if this is indeed a bug. +1 on ByteKeyValueScheme.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1822: STORM-2121: Overriding StringKeyValueScheme.getOutputFiel...
Posted by ikashperskyi <gi...@git.apache.org>.
Github user ikashperskyi commented on the issue:
https://github.com/apache/storm/pull/1822
Now that I think about it we are currently returning a single value tuple with a map so it would make sense to return a pair instead.
@HeartSaVioR @wurstmeister @vesense @asmaier guys what are your thoughts on this?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---