Fence child containers after ConcurrentContainer stops #3377

LokeshAlamuri · 2024-07-19T17:08:50Z

Containers are not restricted from starting after ConcurrentContainer stopped or restarted. These changes would fix this issue.

artembilan · 2024-07-19T17:14:05Z

spring-kafka/src/main/java/org/springframework/kafka/listener/ContainerFencedException.java

+
+/**
+ * Exception throw when container is started when it is not a member of
+ * ConcurrentMessageListenerContainer.


I'm not sure in this conclusion.
The KafkaMessageListenerContainer can be used by itself, without being a member of the ConcurrentMessageListenerContainer.

I think we talked about failing to start a child container when its parent is stopped.
Not sure also if we definitely need a dedicated exception on the matter.
I feel like regular IllegalStateException is fully enough.
Because indeed we are in the illegal state to attempt to start that child container when its parent is stopped.

What do I miss, please?

From design perspective I thought, It would be better if we can provide a specific exception for this scenario. But, IllegalStateException would also works here. I will do that change here if you suggest.

I think new exception would be benefical for the app developers. It improves the error handling. IllegalStateException would work, but it is really generic. From framework perspective, a seperate exception could be better. Please give your comments. I will modify accordingly.

The KafkaMessageListenerContainer can be used by itself, without being a member of the ConcurrentMessageListenerContainer.

I think we talked about failing to start a child container when its parent is stopped.

KafkaMessageListenerContainer can be used individually. Given changes does not modify in this regard. I feel expected spring-kafka system should be as follows.

Assumption: CMain -- ConcurrentContainer. C0, C1 are the child containers. Choosen concurrency 2.

Flow:

CMain -- started.
This would inturn starts C0 and C1.

CMain -- running.
C0 -- running
C1 -- running.

C0 stopped. This should be allowed.

CMain -- not running.
C0 -- not running
C1 -- running.

C0 started. This should be allowed. Since, CMain is not stopped explicitly.

CMain -- running.
C0 -- running
C1 -- running.

C0 stopped. This should be allowed. Since, CMain is not stopped explicitly.

CMain -- not running.
C0 -- not running
C1 -- running.

C1 stopped. This should be allowed. Since, CMain is not stopped explicitly.

CMain -- not running.
C0 -- not running
C1 -- not running.

C0 started. This should be allowed. Since, CMain is not stopped explicitly.

CMain -- not running.
C0 -- running
C1 -- not running.

C1 started. This should be allowed. Since, CMain is not stopped explicitly.

CMain -- running.
C0 -- running
C1 -- running.

CMain -- stopped explicitly.
This would in turn stops C0 and C1. C0 and C1 would be cleared.

CMain -- not running.
C0 -- not running. delinked
C1 -- not running. delinked.

C0 started. This should not be allowed. Since, CMain is stopped explicitly. Container Should be Fenced.

CMain -- started.
This would inturn starts C2 and C3.

CMain -- running.
C2 -- running
C3 -- running.

C0 -- not running. delinked
C1 -- not running. delinked.

The custom exception makes sense if it is intended to be used in the flow control.
In this case it is really abnormal to try to start the container which has not connection with its parent.
Therefore such a logic is wrong and has to be fixed in the target project.
So, IllegalStateException would just do whatever we need to indicated is wrong.

I'm not sure in your use-cases explanation, but looking into behavior one more time, I feel like we have to modify ConcurrentMessageListenerContainer.doStop(final Runnable callback, boolean normal) to call destroy() on all those child containers before this.containers.clear();.
That destroy() should call not only stop(); as it is right now in the MessageListenerContainer, but also mark some flag like destroyed.
Then when we try to call start() on that child container this new flag would give us the mentioned IllegalStateException indicating that container cannot be restarted after being destroyed.

Does that make sense to you?

Thank you for being patient with my review!

Thank you. I have taken the core idea from your comments and modified the logic to solve this issue. I have planned to use containerAllowedToStart flag to verify if the container is allowed to start. Please consider.

I have not used here destroy API for the following reasons.

destroy does not support to take the callback.

A race could occur to start the container immediately after stopped, if destroy is called before clearing the containers and after stopping the containers.

Containers are not restricted from starting after ConcurrentContainer stopped or restarted. These changes would fix this issue.

These changes would use containerAllowedToStart flag to verify if the container is allowed to start rather doing a member check. A new API is added to the MessageListenerContainer. This flag would be set prior to stopping the container to ensure race conditions would not occur start the container once container is stopped immediately.

artembilan

I'm OK with the rest of logic.
Thank you!

artembilan · 2024-07-26T16:58:01Z

spring-kafka/src/main/java/org/springframework/kafka/listener/MessageListenerContainer.java

+	 * @return true if a container is allowed to start.
+	 * @since 2.7.3
+	 */
+	default boolean isAllowedToStart() {


I don't think we need any new public API to complicate a a logic of the target project more.
The child container after stopping from the concurrent one is effectively orphan, therefore it is totally OK to indicate such an illegal state throwing an exception from the doStart() of that container.
If you got that exception, then something is wrong with application logic and it has to be fixed.
Much more robust solution and decision, than trying to if..else it via this new API.

More over that Spring Kafka version is out of support: https://spring.io/projects/spring-kafka#support.
So, even if we will decide to back-port the fix, it won't make it into that EOL version anyway.

More over that Spring Kafka version is out of support: https://spring.io/projects/spring-kafka#support. So, even if we will decide to back-port the fix, it won't make it into that EOL version anyway.

My mistake. I have not update the version to 3.3. As per your comments, I have removed the API.

artembilan · 2024-07-26T17:00:18Z

...fka/src/main/java/org/springframework/kafka/listener/ConcurrentMessageListenerContainer.java

@@ -355,6 +355,7 @@ protected void doStop(final Runnable callback, boolean normal) {
 				}
 			}
 			for (KafkaMessageListenerContainer<K, V> container : this.containers) {
+				container.setAllowedToStart(false);


I would really be more specific here and call it setOrphaned() (or fenced) as you call it in the exception message.

This commit includes following changes. 1. Remove API isAllowedToStart added in previous commit from MessageListenerContainer 2. Rename setAllowedStart to setFenced.

artembilan · 2024-07-29T18:09:03Z

...kafka/src/main/java/org/springframework/kafka/listener/AbstractMessageListenerContainer.java

@@ -732,4 +742,10 @@ protected Properties propertiesFromConsumerPropertyOverrides() {
 		return props;
 	}

+	public boolean isContainerFenced() {


Again: I don't see a reason in any new API like this.
The functionality is totally connected with the start() and if you fail there, then something is off in your logic.
Don't give the framework users to make some choice which would be possible with this new public API.
Just throw that IllegalStateException from the start.
If you meet such an exception, then something has to be fixed in the target project.

artembilan · 2024-07-29T18:11:52Z

...kafka/src/main/java/org/springframework/kafka/listener/AbstractMessageListenerContainer.java

+		this.fenced = fenced;
+	}
+
+	public boolean isFenced() {


I even don't see a reason in this one.
That is fully internal logic of the container architecture.
And if you have to deal with this new public API, then the logic in your project is off.
The consumer of the framework must use it in a proper way.
So, any child containers must be controlled outside of the parent container, when that parent container has abandoned them.

Would you sharing with us some idea what is the use-case that we would need to deal with child lifecycle individually, but not via its parent?

I am not suggesting child container lifecycle has to be managed out side of the parent container lifecycle. It would be highly beneficial in some scenrio's to start and stop the containers individually provided parent container is live. In fact, child container should be no more useful when the parent container has abandoned.

Your comment is correct. APIs are internal and need not be public.

Use case: Assume a highly concurrent spring-kafka application. Let one of the container is stopped for some reason. It would be a nice feature to start the container alone rather than stopping all the containers and starting again.

...kafka/src/main/java/org/springframework/kafka/listener/AbstractMessageListenerContainer.java

artembilan

LGTM.
Will be merged when PR build is green.
Thank you for outstanding contribution!

LokeshAlamuri · 2024-07-31T16:53:32Z

LGTM. Will be merged when PR build is green. Thank you for outstanding contribution!

Thanks a lot!!

LokeshAlamuri mentioned this pull request Jul 19, 2024

Fence the MessageLisnterContainer restart once the ConcurrentMessageListenerContainer is stopped #3371

Closed

artembilan requested changes Jul 19, 2024

View reviewed changes

Lokesh Alamuri added 3 commits July 25, 2024 12:56

Fence child containers after ConcurrentContainer stops

bde3311

Containers are not restricted from starting after ConcurrentContainer stopped or restarted. These changes would fix this issue.

Enhancements to fence container after ConcurrentContainer stops

672b73e

Modify ContainerFencedException documentation

8b78efc

LokeshAlamuri force-pushed the GH-3371 branch from 7319a7d to 8b78efc Compare July 25, 2024 10:37

Lokesh Alamuri added 4 commits July 25, 2024 16:13

Fix checkstyle issue

22baa7e

Fix checkstyle issues

8490f36

Revert changes in KafkaMessageListenerContainer

9f42a1d

artembilan requested changes Jul 26, 2024

View reviewed changes

Remove API to verify if container is fenced

9532495

This commit includes following changes. 1. Remove API isAllowedToStart added in previous commit from MessageListenerContainer 2. Rename setAllowedStart to setFenced.

artembilan requested changes Jul 29, 2024

View reviewed changes

Remove isFenced public API from AbstractMessageListenerContainer

b168cff

artembilan requested changes Jul 30, 2024

View reviewed changes

...kafka/src/main/java/org/springframework/kafka/listener/AbstractMessageListenerContainer.java Show resolved Hide resolved

Remove method to check fenced container

cf391ea

artembilan approved these changes Jul 31, 2024

View reviewed changes

artembilan enabled auto-merge (squash) July 31, 2024 16:54

artembilan merged commit 20696f2 into spring-projects:main Jul 31, 2024
3 checks passed

artembilan mentioned this pull request Aug 6, 2024

Fix ConcurrentContainer lifecycle issues #3406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fence child containers after ConcurrentContainer stops #3377

Fence child containers after ConcurrentContainer stops #3377

LokeshAlamuri commented Jul 19, 2024

artembilan Jul 19, 2024

LokeshAlamuri Jul 19, 2024

LokeshAlamuri Jul 20, 2024

LokeshAlamuri Jul 20, 2024 •

edited

Loading

artembilan Jul 22, 2024

LokeshAlamuri Jul 25, 2024 •

edited

Loading

artembilan left a comment

artembilan Jul 26, 2024

LokeshAlamuri Jul 27, 2024

artembilan Jul 26, 2024

artembilan Jul 29, 2024

artembilan Jul 29, 2024

LokeshAlamuri Jul 30, 2024

artembilan left a comment

LokeshAlamuri commented Jul 31, 2024

Fence child containers after ConcurrentContainer stops #3377

Fence child containers after ConcurrentContainer stops #3377

Conversation

LokeshAlamuri commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LokeshAlamuri Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LokeshAlamuri Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

LokeshAlamuri commented Jul 31, 2024

LokeshAlamuri Jul 20, 2024 •

edited

Loading

LokeshAlamuri Jul 25, 2024 •

edited

Loading