Skip to content

SAMZA-2517 : Adding handling and relevant exception message for null in oldest offset from system-admin#1353

Merged
rmatharu-zz merged 3 commits into
apache:masterfrom
rmatharu-zz:test-csm-npe
May 1, 2020
Merged

SAMZA-2517 : Adding handling and relevant exception message for null in oldest offset from system-admin#1353
rmatharu-zz merged 3 commits into
apache:masterfrom
rmatharu-zz:test-csm-npe

Conversation

@rmatharu-zz
Copy link
Copy Markdown
Contributor

@rmatharu-zz rmatharu-zz commented Apr 29, 2020

Problem: When a Kafka broker returns null for oldest-offset, because of a OpenJDK bug (https://bugs.openjdk.java.net/browse/JDK-8148463), the collect() operation in TaskSideInputStorageManager can fail with a misleading null-pointer-exception message.
Note however that, Kafka broker cannot return null for oldest-offset.

Cause: Above.
Fix: The fix is to use the suggested workaround for the bug https://bugs.openjdk.java.net/browse/JDK-8148463
And update the exception and log messages in case the oldest-offset are returned as null from Kafka.
API changes: None
Upgrade Instructions: None

Comment on lines +375 to +380
Map<SystemStreamPartition, String> offsets = new HashMap<>();
for (SystemStreamPartition ssp : systemStreamToSsp.get(systemStream)) {
if (partitionMetadata == null || partitionMetadata.get(ssp.getPartition()) == null) {
offsets.put(ssp, null);
} else {
offsets.put(ssp, partitionMetadata.get(ssp.getPartition()).getOldestOffset());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This map seems redundant. Why not directly put to oldestOffsets?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

// Because of https://bugs.openjdk.java.net/browse/JDK-8148463 using lambda will NPE when getOldestOffset() is null
Map<SystemStreamPartition, String> offsets = new HashMap<>();
for (SystemStreamPartition ssp : systemStreamToSsp.get(systemStream)) {
if (partitionMetadata == null || partitionMetadata.get(ssp.getPartition()) == null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SystemStreamMetadata#getSystemStreamPartitionMetadata is also used in CoordinatorStreamStore, ContainerStorageManager and other places, we should also do null handling there right?

Isn't it better to do at StreamMetadataCache level?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the null values are on oldestOffset in the returned metadata-object not the returned objectl itself (returned val is by us and is not-null it appears).

@bkonold
Copy link
Copy Markdown
Contributor

bkonold commented Apr 30, 2020

@rmatharu the change lgtm once the minor comment on unit test is resolved

@prateekm
Copy link
Copy Markdown
Contributor

@rmatharu-zz rmatharu-zz merged commit 9977adf into apache:master May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants