Skip to content

CAMEL-17489: camel-kafka - Unsubscribe before closing the consumer#6751

Merged
orpiske merged 1 commit intoapache:mainfrom
rgala:main
Jan 14, 2022
Merged

CAMEL-17489: camel-kafka - Unsubscribe before closing the consumer#6751
orpiske merged 1 commit intoapache:mainfrom
rgala:main

Conversation

@rgala
Copy link
Contributor

@rgala rgala commented Jan 14, 2022

No description provided.

// only close if not retry
if (!isRetrying()) {
LOG.debug("Closing consumer {}", threadId);
safeUnsubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

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

An exception here might trigger the exception handler ... but I guess it's OK.

@davsclaus
Copy link
Contributor

I think safeUnsubscribe should be made safe and deal with already closed, eg kafka throws this

    private void acquireAndEnsureOpen() {
        acquire();
        if (this.closed) {
            release();
            throw new IllegalStateException("This consumer has already been closed.");
        }
    }

eg make safeUnsubscrive catch IllegalStateException and ignore that

@orpiske
Copy link
Contributor

orpiske commented Jan 14, 2022

I think safeUnsubscribe should be made safe and deal with already closed, eg kafka throws this

    private void acquireAndEnsureOpen() {
        acquire();
        if (this.closed) {
            release();
            throw new IllegalStateException("This consumer has already been closed.");
        }
    }

eg make safeUnsubscrive catch IllegalStateException and ignore that

Yeah, I think it may be OK to ignore the IllegalStateException in this specific case. I think there are other issues that could happen during unsubscribe, so we need to be careful to ignore only that IMHO.

@orpiske orpiske merged commit 3a70856 into apache:main Jan 14, 2022
@orpiske
Copy link
Contributor

orpiske commented Jan 14, 2022

I am merging this, and then I'll improve the safeUnsubscribe.

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.

3 participants