Delete redundant null check

[ghgh415263]

Expected Behavior
null check once

Current Behavior
check twice

Context
DefaultKafkaProducerFactory class
method abortTransaction()
There are two null checks on the producerFailed variable.

		@Override
		public void abortTransaction() throws ProducerFencedException {
			LOGGER.debug(() -> toString() + " abortTransaction()");
			if (this.producerFailed != null) {
				LOGGER.debug(() -> {
					String message = this.producerFailed == null ? "" : this.producerFailed.getMessage();
					return "abortTransaction ignored - previous txFailed: " + message
							+ ": " + this;
				});
			}
			else {
				try {
					this.delegate.abortTransaction();
				}
				catch (RuntimeException e) {
					LOGGER.error(e, () -> "Abort failed: " + this);
					this.producerFailed = e;
					throw e;
				}
			}
		}

What do you think about changing it like this?

if (this.producerFailed != null) {
   LOGGER.debug(() -> "abortTransaction ignored - previous txFailed: " + this.producerFailed.getMessage()
            + ": " + this);
}

thanks for reading.

[sobychacko]

@ghgh415263 That sounds reasonable. Please submit a PR with that.

[artembilan]

That this.producerFailed property can be modified from different threads in other methods.
So, I think your catch is good, but the fix has to be like extracting that this.producerFailed into a local variable before if.

[artembilan]

I still treat it as a bug, since we cannot be sure about the property state in multi-threaded environment.
So, or sync barrier, or local variable.

[ghgh415263]

so, local variable. like

Exception producerFailed = this.producerFailed;
LOGGER.debug(() -> toString() + " abortTransaction()");
if (producerFailed != null) {
	LOGGER.debug(() -> "abortTransaction ignored - previous txFailed: " + producerFailed.getMessage()
		+ ": " + this);
}

or lock?

Which approach do you think is better?

[artembilan]

No lock, please.
There is really no reason to have a lock around this simple property.
Yes, just copy into local variable and that's it.
However you cannot use the same name: it hides the real property with all possible consequences: https://stackoverflow.com/questions/29832466/what-is-the-meaning-of-local-variable-hides-a-field
So, something like:

Exception producerFailedToUse = this.producerFailed;

would do the proper work.

[ghgh415263]

Thank you for reviewing my changes. I have made the updates in the new commit. Would it be alright if I proceed with sending the pull request?

[artembilan]

Yes, we can review your contribution only from PRs.
Thanks