STORM-2121: Overriding StringKeyValueScheme.getOutputFields to contain both key and value

[ikashperskyi]

Added proper fields to getOutputFields method of StringKeyValueScheme as described in https://issues.apache.org/jira/browse/STORM-2121.
Minor refactoring of deserializeKeyAndValue method: inlined deserialization and added static import for StringScheme.deserializeString.

[HeartSaVioR]

+1

[vesense]

+1

[HeartSaVioR]

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?

[ikashperskyi]

@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.

[ikashperskyi]

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?

[HeartSaVioR]

@ikashperskyi I would like to also address this to 1.x so keeping backward compatibility is ideal for now.

[harshach]

@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.

[ikashperskyi]

@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.