Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

Conversation

@patilvikram
Copy link
Contributor

No description provided.

@pramodin
Copy link
Contributor

Is it possible to come up with a unit test for this. Looks like the NPE happens only in some cases.

@pramodin
Copy link
Contributor

There are checkstyle errors, please see

[ERROR] src/main/java/org/apache/apex/malhar/lib/wal/FSWindowDataManager.java:[487,11] (whitespace) WhitespaceAround: WhitespaceAround: 'if' is not followed by whitespace.
[ERROR] src/main/java/org/apache/apex/malhar/lib/wal/FSWindowDataManager.java:[488] (indentation) Indentation: 'reader' have incorrect indentation level 11, expected level should be 12.

This looks like an edge case as this code generally works and doesn't error always. If it is an edge case and you are unable to come up with a unit test due to that reason, if there is no objection from others, I am ok to merge it without a unit test in this case.

@vrozov
Copy link
Member

vrozov commented May 29, 2019

I am not fully convinced that this is the right fix without a unit test. Looking at readNext() implementation, I would suspect that null is returned when the next record was not fully written or there is actually I/O problem. It is not clear, why is it OK to suppress I/O exception in readNext() instead of propagating it as retrieve() signature suggests. For the incomplete written record, it should be possible to create a unit test and see what should be the correct behavior.

if (wal.retrievedWindow == null) {
wal.retrievedWindow = readNext(reader);
if(wal.retrievedWindow == null &&
reader.getCurrentPointer().compareTo(wal.walEndPointerAfterRecovery) < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

What if wal.retrievedWindow == null && reader.getCurrentPointer().compareTo(wal.walEndPointerAfterRecovery) >= 0?

reader.getCurrentPointer().compareTo(wal.walEndPointerAfterRecovery) < 0) {
continue;
}
Preconditions.checkNotNull(wal.retrievedWindow);
Copy link
Member

Choose a reason for hiding this comment

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

There is an assumption that wal.retrievedWindow != null that does not seems to be valid.

@pramodin
Copy link
Contributor

pramodin commented Jun 3, 2019

@patilvikram can you respond to @vrozov comments

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants