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

Remove AbstractMessageListenerContainer.isPaused #3016

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

Wzy19930507
Copy link
Contributor

Use AbstractMessageListenerContainer method isPauseRequested uniformly instead of isPaused().

@Wzy19930507
Copy link
Contributor Author

I executed ./gradlew check again and executed TestOOMError multiple times, but can not reproduce the error in the image
image

@sobychacko
Copy link
Contributor

I am not sure whether we should remove the isPaused method from the container API as it makes more sense than the isPauseRequested counterpart.

@Wzy19930507
Copy link
Contributor Author

Wzy19930507 commented Feb 6, 2024

isPaused() use to adaptor resumed/paused, see spring-integration-kafka#198, after #609 container has two phase for paused/resume that isPauseRequested and isContainerPaused.

see #609 comment about isPaused()

it doesn't really reflect whether the container is actually paused yet, just that pause() has been called (it takes a little while before the actual pause occurs).

look like same as

	/**
	 * Return true if {@link #pause()} has been called; the container might not have actually
	 * paused yet.
	 * @return true if pause has been requested.
	 * @since 2.1.5
	 */
	default boolean isPauseRequested() {
		throw new UnsupportedOperationException("This container doesn't support pause/resume");
	}

@artembilan
Copy link
Member

Yes. I totally agree with the change.

@sobychacko ,
please, proceed with the detailed review.

I'm not sure how that ConcurrentModificationException is possible: we have all the this.containers guarded by the this.lifecycleLock.lock() and I don't see any modifications from the outside in that test...

@sobychacko
Copy link
Contributor

Looks like the tests are green now. I will do another pass of the review. Don't you think the isPaused removal deprecated first since its protected?

@artembilan
Copy link
Member

I think the code is now like this in this PR:

@Deprecated(since = "3.2", forRemoval = true)
protected boolean isPaused() {

So, we are good.

@sobychacko
Copy link
Contributor

Sorry, I missed that.

@sobychacko sobychacko merged commit 74f6882 into spring-projects:main Feb 6, 2024
3 checks passed
@Wzy19930507 Wzy19930507 deleted the remove_ispaused branch February 12, 2024 16:43
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

3 participants