You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "hanicz (via GitHub)" <gi...@apache.org> on 2025/03/28 16:57:15 UTC
[PR] KNOX-3115: HA Dispatch for SSE [knox]
hanicz opened a new pull request, #1010:
URL: https://github.com/apache/knox/pull/1010
## What changes were proposed in this pull request?
[KNOX-3065](https://issues.apache.org/jira/projects/KNOX/issues/KNOX-3065) Introduced the proper support for SSE connections. However HA Dispatch is missing. This PR add a new SSEHaDispatch that is able to load balance and use sticky session cookies. Failover is not added yet. The SSEHaDispatch is based on ConfigurableHaDispatch however it can't extend it due to extending SSEDispatch instead.
## How was this patch tested?
Unit tests.
Local manual tests with a custom SSE service.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #1010:
URL: https://github.com/apache/knox/pull/1010#issuecomment-2782485017
I gave the new version a quick review, and it looks good to me. I'd like to as @pzampino and @moresandeep to review the code again.
I'd also like to ask you, @hanicz, to elaborate on your manual testing in the PR description. Since your changes now have an effect on the 'usual' HA dispatch too, I'd be interested to see one (or more) service(s) testing from the HA perspective. It's perfectly enough to indicate you tested XYZ service with its HA-dispatch configured and failover/retry happened as expected. I'd just like to see some proof we did that before merging this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #1010:
URL: https://github.com/apache/knox/pull/1010#issuecomment-2774733334
> > This seems to duplicate some common HaDispatch code. I haven't closely enough to determine how much could be re-used here instead.
>
> I can introduce a new interface to remove some duplication with default methods.
That would be nice, @hanicz !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "pzampino (via GitHub)" <gi...@apache.org>.
pzampino commented on code in PR #1010:
URL: https://github.com/apache/knox/pull/1010#discussion_r2019096651
##########
gateway-provider-ha/src/test/java/org/apache/knox/gateway/ha/dispatch/SSEHaDispatchTest.java:
##########
@@ -0,0 +1,330 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.ha.dispatch;
+
+import org.apache.http.HttpStatus;
+import org.apache.http.client.config.CookieSpecs;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.ha.provider.HaDescriptor;
+import org.apache.knox.gateway.ha.provider.HaProvider;
+import org.apache.knox.gateway.ha.provider.impl.DefaultHaProvider;
+import org.apache.knox.gateway.ha.provider.impl.HaDescriptorFactory;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.ServiceType;
+import org.apache.knox.gateway.services.security.KeystoreService;
+import org.apache.knox.test.mock.MockServer;
+import org.apache.knox.test.mock.MockServletContext;
+import org.apache.knox.test.mock.MockServletInputStream;
+import org.easymock.Capture;
+import org.easymock.EasyMock;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import javax.servlet.AsyncContext;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletContext;
+import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.io.PrintWriter;
+import java.net.URI;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.easymock.EasyMock.capture;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class SSEHaDispatchTest {
+
+ private static MockServer MOCK_SSE_SERVER;
+ private static URI URL;
+
+ @BeforeClass
+ public static void setUpBeforeClass() throws Exception {
+ MOCK_SSE_SERVER = new MockServer("SSE", true);
Review Comment:
Would you not need two servers to test HA?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 merged PR #1010:
URL: https://github.com/apache/knox/pull/1010
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on code in PR #1010:
URL: https://github.com/apache/knox/pull/1010#discussion_r2019133574
##########
gateway-provider-ha/src/test/java/org/apache/knox/gateway/ha/dispatch/SSEHaDispatchTest.java:
##########
@@ -0,0 +1,330 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.ha.dispatch;
+
+import org.apache.http.HttpStatus;
+import org.apache.http.client.config.CookieSpecs;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.ha.provider.HaDescriptor;
+import org.apache.knox.gateway.ha.provider.HaProvider;
+import org.apache.knox.gateway.ha.provider.impl.DefaultHaProvider;
+import org.apache.knox.gateway.ha.provider.impl.HaDescriptorFactory;
+import org.apache.knox.gateway.services.GatewayServices;
+import org.apache.knox.gateway.services.ServiceType;
+import org.apache.knox.gateway.services.security.KeystoreService;
+import org.apache.knox.test.mock.MockServer;
+import org.apache.knox.test.mock.MockServletContext;
+import org.apache.knox.test.mock.MockServletInputStream;
+import org.easymock.Capture;
+import org.easymock.EasyMock;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import javax.servlet.AsyncContext;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletContext;
+import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.io.PrintWriter;
+import java.net.URI;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.easymock.EasyMock.capture;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class SSEHaDispatchTest {
+
+ private static MockServer MOCK_SSE_SERVER;
+ private static URI URL;
+
+ @BeforeClass
+ public static void setUpBeforeClass() throws Exception {
+ MOCK_SSE_SERVER = new MockServer("SSE", true);
Review Comment:
Not necessarily, just two different endpoints. The test is checking the activeURL after the first call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on PR #1010:
URL: https://github.com/apache/knox/pull/1010#issuecomment-2762107580
> This seems to duplicate some common HaDispatch code. I haven't closely enough to determine how much could be re-used here instead.
I can introduce a new interface to remove some duplication with default methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on PR #1010:
URL: https://github.com/apache/knox/pull/1010#issuecomment-2785591919
> I gave the new version a quick review, and it looks good to me. I'd like to ask @pzampino and @moresandeep to review the code again.
>
> I'd also like to ask you, @hanicz, to elaborate on your manual testing steps in the PR description. Since your changes now have an effect on the 'usual' HA dispatch too, I'd be interested to see one (or more) service(s) testing from the HA perspective. It's perfectly enough to indicate you tested XYZ service with its HA-dispatch configured and failover/retry happened as expected. I'd just like to see some proof we did that before merging this PR.
@smolnar82 I updated the the PR description with my testing method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3115: HA Dispatch for SSE [knox]
Posted by "hanicz (via GitHub)" <gi...@apache.org>.
hanicz commented on PR #1010:
URL: https://github.com/apache/knox/pull/1010#issuecomment-2782393432
> > > This seems to duplicate some common HaDispatch code. I haven't closely enough to determine how much could be re-used here instead.
> >
> >
> > I can introduce a new interface to remove some duplication with default methods.
>
> That would be nice, @hanicz !
I introduced a new interface to remove the code duplication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org