Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pulsar-io] Refine the key in redis sink when key is null #11192

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

murong00
Copy link
Contributor

@murong00 murong00 commented Jul 2, 2021

Motivation

When the record has a null key, someone maybe confused by a NPE warn as below:

11:26:22.730 [pool-5-thread-1] WARN  org.apache.pulsar.io.redis.sink.RedisSink - Record flush thread was exception
java.lang.NullPointerException: null
        at java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1011) ~[?:1.8.0_291]
        at java.util.concurrent.ConcurrentHashMap.put(ConcurrentHashMap.java:1006) ~[?:1.8.0_291]
        at org.apache.pulsar.io.redis.sink.RedisSink.flush(RedisSink.java:127) ~[pulsar-io-redis-2.8.0.nar-unpacked/:?]
        at org.apache.pulsar.io.redis.sink.RedisSink.lambda$write$1(RedisSink.java:94) ~[pulsar-io-redis-2.8.0.nar-unpacked/:?]

We can use an empty string as key when its key is null as other sinks do.

Modifications

  • Use an empty string as key when its key is null.

@murong00 murong00 self-assigned this Jul 2, 2021
byte[] value = record.getValue();
byte[] key = record.getKey().isPresent() ? record.getKey().get().getBytes(StandardCharsets.UTF_8) : value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you using the value as key ?

probably if the record has no "key" we should discard it
mapping a null key to the value is not immediate for the user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A null key cannot flush to redis, java.lang.NullPointerException: null may confuse someone who will treat as no record is coming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an empty string "" looks better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty String makes more sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please take a look again, thanks.

Copy link
Member

@nlu90 nlu90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior looks strange to me.

Could you provide some link to other connectors that use value as key?

@murong00
Copy link
Contributor Author

murong00 commented Jul 5, 2021

This behavior looks strange to me.

Could you provide some link to other connectors that use value as key?

Like CassandraStringSink, AerospikeStringSink and HdfsTextSink using record.getKey().orElseGet(() -> record.getValue()) as key.

@nlu90
Copy link
Member

nlu90 commented Jul 6, 2021

I think this is kind of the standard behavior for the connectors regarding this PR from @merlimat #2116

I'm okay with it.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eolivelli
Copy link
Contributor

@congbobo184 @codelipenghui PTAL
we need another binding +1

@sijie sijie added this to the 2.9.0 milestone Jul 14, 2021
@sijie sijie merged commit 27797d9 into apache:master Jul 14, 2021
codelipenghui pushed a commit that referenced this pull request Jul 23, 2021
### Motivation

When the record has a null key, someone maybe confused by a NPE warn as below:

```
11:26:22.730 [pool-5-thread-1] WARN  org.apache.pulsar.io.redis.sink.RedisSink - Record flush thread was exception
java.lang.NullPointerException: null
        at java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1011) ~[?:1.8.0_291]
        at java.util.concurrent.ConcurrentHashMap.put(ConcurrentHashMap.java:1006) ~[?:1.8.0_291]
        at org.apache.pulsar.io.redis.sink.RedisSink.flush(RedisSink.java:127) ~[pulsar-io-redis-2.8.0.nar-unpacked/:?]
        at org.apache.pulsar.io.redis.sink.RedisSink.lambda$write$1(RedisSink.java:94) ~[pulsar-io-redis-2.8.0.nar-unpacked/:?]
```

We can use an empty string as key when its key is null as other sinks do.

### Modifications

- Use an empty string as key when its key is null.

(cherry picked from commit 27797d9)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 23, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
### Motivation

When the record has a null key, someone maybe confused by a NPE warn as below:

```
11:26:22.730 [pool-5-thread-1] WARN  org.apache.pulsar.io.redis.sink.RedisSink - Record flush thread was exception
java.lang.NullPointerException: null
        at java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1011) ~[?:1.8.0_291]
        at java.util.concurrent.ConcurrentHashMap.put(ConcurrentHashMap.java:1006) ~[?:1.8.0_291]
        at org.apache.pulsar.io.redis.sink.RedisSink.flush(RedisSink.java:127) ~[pulsar-io-redis-2.8.0.nar-unpacked/:?]
        at org.apache.pulsar.io.redis.sink.RedisSink.lambda$write$1(RedisSink.java:94) ~[pulsar-io-redis-2.8.0.nar-unpacked/:?]
```

We can use an empty string as key when its key is null as other sinks do.

### Modifications

- Use an empty string as key when its key is null.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants