PIP-1 - Introduce Pulsar proxy component #548

merlimat · 2017-07-06T01:16:47Z

Motivation

https://github.com/apache/incubator-pulsar/wiki/PIP-1:-Pulsar-Proxy

As described in the wiki page, added a proxy component that exposes a stateless service that talk Pulsar binary protocol.

rdhabalia · 2017-07-07T18:03:05Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/BinaryProtoLookupService.java

@@ -59,7 +62,7 @@ public BinaryProtoLookupService(PulsarClientImpl client, String serviceUrl, bool
     * @param destination: topic-name
     * @return broker-socket-address that serves given topic
     */
-    public CompletableFuture<InetSocketAddress> getBroker(DestinationName destination) {
+    public CompletableFuture<Pair<InetSocketAddress, InetSocketAddress>> getBroker(DestinationName destination) {


do you think using InetSocketAddress[] would be better to avoid creation of Pair object here.

In this case it would be the same number of object allocations, either a Pair<> or the InetSocketAddress[]. No differences, except Pair conveys better the fact that 2 addresses are going to be there.

yes, I meant avoiding creation of Pair.

conveys better the fact that 2 addresses are going to be there.

Yes, make sense.

rdhabalia · 2017-07-07T18:19:54Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java

@@ -77,6 +78,8 @@
    private final int maxNumberOfRejectedRequestPerConnection;
    private final int rejectedRequestResetTimeSec = 60;

+    private String targetBrokerAddress = null;


do you think if we can add proxy in the name it will easy to understand.

Sure, will rename that

rdhabalia · 2017-07-07T18:22:52Z

pulsar-common/src/main/proto/PulsarApi.proto

@@ -188,6 +197,11 @@ message CommandLookupTopicResponse {
 	optional bool authoritative           = 5 [default = false];
 	optional ServerError error            = 6;
 	optional string message               = 7;
+
+    // If it's true, indicates to the client that it must


rdhabalia · 2017-07-07T18:42:35Z

pulsar-common/src/main/proto/PulsarApi.proto

+	// Original principal that was verified by
+	// a Pulsar proxy. In this case the auth info above
+	// will the the auth of the proxy itself
+	optional string original_principal = 7;


if proxy is going to do auth&auth for original-principal then broker will not do any validation on this original_principal right? if that's true then are we passing it for debugging only? and I don't see if we are setting it in Commands.java

Proxy is not doing authorization on the data connections. The reason is that after the initial connect/connected it just forward everything on both sides.

The broker still needs to check whether that particular role is authorized to pulblish on a specific topic. So the proxy relays the original client principal, and that will be used by the broker for the authorization, trusting that the proxy has already validated the authentication.

rdhabalia · 2017-07-07T18:48:29Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java

+            if (future.isSuccess()) {
+                outboundChannel.read();
+            } else {
+                inboundChannel.close();


should we log warn message here?

rdhabalia · 2017-07-07T18:56:15Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java

+        public void operationComplete(Future<Void> future) throws Exception {
+            // This is invoked when the write operation on the paired connection is completed
+            if (future.isSuccess()) {
+                outboundChannel.read();


but we have already triggered read on outboundChannel on line-107, so channel-handler will continue reading even if we don't call here. right?

No, the auto-read on the channel is set to false. The reason is to adapt to the speed of the slowest connection.

So, we read 1 buffer from c1, write into c2. When the write is completed (meaning we were able to write on the socket because the TCP send window was open), then we read the next buffer.

rdhabalia · 2017-07-07T19:01:29Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/LookupProxyHandler.java

+            brokerURI = new URI(brokerServiceUrl);
+        } catch (URISyntaxException e) {
+            proxyConnection.ctx().writeAndFlush(
+                    Commands.newLookupErrorResponse(ServerError.ServiceNotReady, e.getMessage(), clientRequestId));


ServerError.MetadataError?

Makes more sense. Fixed

rdhabalia · 2017-07-07T19:07:29Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java

+    }
+
+    private void close() {
+        ctx.close();


should we have additional state= 'ConnectionClosed` ?

merlimat · 2017-07-10T21:56:31Z

@rdhabalia Addressed comments

rdhabalia

LGTM .. just minor comments.

rdhabalia · 2017-07-11T02:44:04Z

pulsar-proxy/src/main/resources/proxy.conf

+#
+
+# Zookeeper quorum connection string (comma-separated)
+zookeeperServers=127.0.0.1:2181


in other conf file we have kept it empty. should we do the same?

Yes, I forgot these after testing

rdhabalia · 2017-07-11T02:49:00Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/util/ZookeeperCacheLoader.java

+    private final ZooKeeperCache localZkCache;
+    private final LocalZooKeeperConnectionService localZkConnectionSvc;
+
+    private final ZooKeeperDataCache<LoadReport> brokerInfo;


org.apache.pulsar.policies.data.loadbalancer.ServiceLookupData because ModularLoadManager generates LocalBrokerData.

rdhabalia · 2017-07-11T02:50:42Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java

+        DefaultExports.initialize();
+        server.addServlet("/metrics", new ServletHolder(MetricsServlet.class));
+
+        // server.addServlet("/*", DiscoveryServiceServlet.class, initParameters);


does this comment require for any indication?

forgot this, will remove

rdhabalia · 2017-07-11T02:58:58Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

+        this.serviceUrlTls = String.format("pulsar://%s:%d/", hostname, proxyConfig.getServicePortTls());
+
+        EventLoopGroup acceptorEventLoop, workersEventLoop;
+        if (SystemUtils.IS_OS_LINUX) {


just a thought SystemUtils.IS_OS_LINUX && io.netty.channel.epoll.Epoll.isAvailable()?

I didn't know about Epoll.isAvailable(), probably was introduced later in Netty. Then we can just use that check, no need to verify we are on Linux.

merlimat · 2017-07-11T05:47:29Z

@rdhabalia Updated

rdhabalia

👍

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Jul 6, 2017

merlimat added this to the 1.19.0-incubating milestone Jul 6, 2017

merlimat self-assigned this Jul 6, 2017

merlimat requested review from rdhabalia and saandrews July 6, 2017 01:16

PIP-1 - Introduce Pulsar proxy component

bed33fd

merlimat force-pushed the proxy branch from 866482a to bed33fd Compare July 6, 2017 17:39

rdhabalia reviewed Jul 7, 2017

View reviewed changes

merlimat added 2 commits July 10, 2017 14:33

Fixed formatting in proto file

a2f7ba6

Addressed PR comments

53e9f7e

rdhabalia reviewed Jul 11, 2017

View reviewed changes

Addressed comments

aaf5ee2

rdhabalia approved these changes Jul 11, 2017

View reviewed changes

merlimat added 2 commits July 10, 2017 23:10

Fixed jcommander dependency

ef27c26

Fixed ProducerConsumer proxy test

0979a36

merlimat merged commit f36b70c into apache:master Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-1 - Introduce Pulsar proxy component #548

PIP-1 - Introduce Pulsar proxy component #548

merlimat commented Jul 6, 2017 •

edited

Loading

rdhabalia Jul 7, 2017

merlimat Jul 10, 2017

rdhabalia Jul 11, 2017

rdhabalia Jul 7, 2017

merlimat Jul 10, 2017

rdhabalia Jul 7, 2017

rdhabalia Jul 7, 2017

merlimat Jul 8, 2017

rdhabalia Jul 7, 2017

merlimat Jul 10, 2017

rdhabalia Jul 7, 2017

merlimat Jul 8, 2017

rdhabalia Jul 7, 2017

merlimat Jul 10, 2017

rdhabalia Jul 7, 2017

merlimat Jul 10, 2017

merlimat commented Jul 10, 2017

rdhabalia left a comment

rdhabalia Jul 11, 2017

merlimat Jul 11, 2017

rdhabalia Jul 11, 2017

merlimat Jul 11, 2017

rdhabalia Jul 11, 2017

merlimat Jul 11, 2017

rdhabalia Jul 11, 2017 •

edited

Loading

merlimat Jul 11, 2017

merlimat commented Jul 11, 2017

rdhabalia left a comment

PIP-1 - Introduce Pulsar proxy component #548

PIP-1 - Introduce Pulsar proxy component #548

Conversation

merlimat commented Jul 6, 2017 • edited Loading

Motivation

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Jul 10, 2017

rdhabalia 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

rdhabalia Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Jul 11, 2017

rdhabalia left a comment

Choose a reason for hiding this comment

merlimat commented Jul 6, 2017 •

edited

Loading

rdhabalia Jul 11, 2017 •

edited

Loading