Support Topic lookup using Pulsar binary protocol #5

rdhabalia · 2016-09-10T00:30:41Z

Motivation

To improve lookup performance: adding lookup API in Pulsar binary protocol for:

PartitionedTopicMetadata
Topic-lookup: find BrokerService-Url

Modifications

Introduced two new protobuf-commands for partitionedMetadata and topic lookup
server/client api to handle lookup request/response

Result

Result: Client-library can do topic-lookup using pulsar binary protocol.

yahoocla · 2016-09-10T00:30:42Z

CLA is valid!

merlimat · 2016-09-10T01:13:43Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/namespace/NamespaceService.java

+            }
+        });
+
+        LookupResult result = new LookupResult(new URI(candidateBroker), report.getPulsarServiceUrl(), report.getPulsarServieUrlTls());


I think we should use the http & https urls from the report here (all 4 the urls)

Actually in this case, returned LookupResult contains redirect urls.

Http lookup request just requires broker-url where call should be redirected. So, LookupResult has one http/https url

Binary-Protocol request sends both broker service-url/tls and client decides which one needs to be selected based on client configuration.

merlimat · 2016-09-12T20:02:39Z

pulsar-client/src/main/java/com/yahoo/pulsar/client/impl/PulsarClientImpl.java

@@ -56,7 +56,8 @@
    private static final Logger log = LoggerFactory.getLogger(PulsarClientImpl.class);

    private final ClientConfiguration conf;
-    private final HttpClient httpClient;
+    private HttpClient httpClient;
+    private boolean isHttpURI;


We should abstract the LookupService to have 2 different implementation instead of relying on the flag.

yes.. made the change to abstract LookupService.

sboobna · 2016-09-15T17:15:41Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java

@@ -177,7 +177,8 @@ public SimpleLoadManagerImpl(PulsarService pulsar) {
        this.realtimeResourceQuotas.set(new HashMap<>());
        this.realtimeAvgResourceQuota = new ResourceQuota();
        placementStrategy = new WRRPlacementStrategy();
-        lastLoadReport = new LoadReport();
+        lastLoadReport = new LoadReport(pulsar.getWebServiceAddress(), pulsar.getWebServiceAddressTls(),


This change is already in the other PR #11

yes..we picked up the same commit to unblock this enhancement.

That PR is merged, so you can just rebase to get rid of the commit from this PR.

sboobna · 2016-09-15T17:19:41Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/lookup/DestinationLookup.java

+     * 
+     * Lookup broker-service address for a given namespace-bundle which contains given topic.
+     *  
+     * a. If current-broker receives lookup-request and if it's not a leader


Don't we return the broker address if the bundle is already owned? If I'm not wrong, we only redirect the request if the bundle is not owned.

yes.. that is correct. added that point in comment as well.

sboobna · 2016-09-15T17:26:47Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/lookup/DestinationLookup.java

+            String clientAppId, long requestId) {
+        final String cluster = fqdn.getCluster();
+        try {
+            // (1) validate cluster


can we put these validation checks in separate methods maybe?

actually, we have already abstracted all validation. and each validation step: we are returning different LookupResponse-ByteBuf

sboobna · 2016-09-15T17:29:04Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/lookup/LookupResult.java

    }

    private final Type type;
    private final LookupData lookupData;
-    private final URI httpRedirectAddress;


Could you confirm if removing this will be backward compatible?

yes.. as we were not returning httpRedirectAddress to client and it was being used in internal method to find out final returning result.

If Type== HttpRedirectUrl then redirect-http request to URL httpRedirectAddress.

However, in new change all urls service/web-service/tls will be part of LookupData and redirection will be determined using RedirectUrl type.

sboobna · 2016-09-15T17:55:05Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/namespace/NamespaceService.java

@@ -366,7 +357,8 @@ private LookupResult findBrokerServiceUrl(ServiceUnitId suName, boolean authorit
                            pulsar.getBrokerServiceUrl(), pulsar.getWebServiceAddress(), candidateBroker, suName);
                }
                // Now setting the redirect url
-                return new LookupResult(new URI(candidateBroker));
+                //new LookupResult(new URI(candidateBroker))


Please remove this line

removed comment..

sboobna · 2016-09-15T17:56:28Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/namespace/NamespaceService.java

+        return result;
+    }
+
+    public ObjectMapper jsonMapper() {


This method is already there in AdminResource.java

sboobna · 2016-09-15T17:57:32Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/ServerCnx.java

+    }
+
+    @Override
+    protected void handlePartitionRequest(CommandPartitionedTopicMetadata partitionMetadata) {


handlePartitionRequest -> handlePartitionMetadataRequest

alright.. renamed the method name.

sboobna · 2016-09-15T18:04:20Z

pulsar-client/src/main/java/com/yahoo/pulsar/client/impl/BinaryProtoLookupService.java

+    	return serviceAddress.toString();
+    }
+
+    static class LookupDataResult {


Shouldn't we reuse the POJO that we already have for LookupResult

LookupResult POJO is being used to return Lookup result by Broker-http-lookup. However, to implement Lookup-Response-handling in binary-protocol lookup we need few more additional attributes such as authoritative, redirect, partitions. So, we have this POJO at client side to handle Future.Response for topic-lookup and partition-lookup

rdhabalia · 2016-10-31T17:51:51Z

Please ignore travis-build for this PR for now: as it seems there is an issue with travis-build, it always picks up a wrong commit-1b99fa608daa42955e09356f89ae498d6c969347 from different PR and that wrong commit had a unit-testcase-failure-issue which is already fixed in later commit of PR. I have local-travis-job setup which is green by picking up correct commit.

merlimat · 2016-10-28T19:05:36Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/namespace/NamespaceService.java

+                    uri.getPort());
+            pulsar.getLocalZkCache().getDataAsync(path, new Deserializer<LoadReport>() {
+                @Override
+                public LoadReport deserialize(String key, byte[] content) throws Exception {


This deserializer could be made static

merlimat · 2016-10-28T19:10:22Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/ServerCnx.java

+                lookup.getAuthoritative(), getRole(), lookup.getRequestId()).thenAccept(lookupResponse -> {
+                    ctx.writeAndFlush(lookupResponse);
+                }).exceptionally(ex -> {
+                    // it should never happen


This could actually fail for multiple reasons :) (invalid auth, no zk session.. etc )

Actually, lookupDestinationAsync(..) handles all failure scenarios (auth, no zk session,..) and always returns appropriate ByteBuf with specific error-response. therefore, future should not complete with exception.

I have addressed all other comments as well.

merlimat · 2016-10-28T19:12:49Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/web/PulsarWebResource.java

@@ -128,7 +136,7 @@ protected void validateSuperUserAccess() {
        if (config().isAuthenticationEnabled()) {
            String appId = clientAppId();
            log.debug("[{}] Check super user access: Authenticated: {} -- Role: {}", uri.getRequestUri(),


Can you wrap this log.debug() call with the if (log.isDebugEnabled())

merlimat · 2016-10-28T20:02:54Z

pulsar-client/src/main/java/com/yahoo/pulsar/client/impl/BinaryProtoLookupService.java

+class BinaryProtoLookupService implements LookupService {
+
+    private final ConnectionPool cnxPool;
+    protected InetSocketAddress serviceAddress;


serviceAddress can be final

merlimat · 2016-11-09T20:27:19Z

@rdhabalia There seems to be a conflict with master in the latest push

merlimat

👍

* add protobuf.json to git * make protobuf doc generation static

) *Motivation* Use a customized version slows down the development. *Modifications* - Change to use reflection to access private fields and methods - Add license headers - Delete unneeded filees *Verify this change* - Enable CI

rdhabalia changed the title ~~CMS-1920 TCP: PartitionMetadata-Lookup + Topic-Broker lookup~~ TCP: Topic lookup Sep 10, 2016

rdhabalia added this to the 1.15 milestone Sep 10, 2016

rdhabalia self-assigned this Sep 10, 2016

rdhabalia added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Sep 10, 2016

rdhabalia changed the title ~~TCP: Topic lookup~~ Support Topic lookup using Pulsar binary protocol Sep 10, 2016

rdhabalia force-pushed the lookup branch from 5b6c259 to fc6fa39 Compare September 10, 2016 01:09

merlimat reviewed Sep 10, 2016
View reviewed changes

rdhabalia force-pushed the lookup branch 2 times, most recently from e254af3 to ff3c89b Compare September 12, 2016 17:38

merlimat reviewed Sep 12, 2016
View reviewed changes

rdhabalia force-pushed the lookup branch 8 times, most recently from e5dcb7a to f37db88 Compare September 15, 2016 01:34

sboobna reviewed Sep 15, 2016

View reviewed changes

rdhabalia force-pushed the lookup branch 7 times, most recently from d6b782e to 76094a4 Compare September 20, 2016 01:24

merlimat modified the milestones: 1.16, 1.15 Oct 8, 2016

rdhabalia force-pushed the lookup branch from 588fa35 to 7014739 Compare October 31, 2016 02:34

merlimat reviewed Oct 31, 2016

View reviewed changes

rdhabalia force-pushed the lookup branch from 7014739 to f2c0104 Compare October 31, 2016 21:17

Support Topic lookup using Pulsar binary protocol

96c1299

rdhabalia force-pushed the lookup branch from f2c0104 to 96c1299 Compare November 9, 2016 22:15

rdhabalia mentioned this pull request Nov 9, 2016

Support Topic lookup using Pulsar binary protocol #105

Closed

merlimat approved these changes Nov 9, 2016

View reviewed changes

merlimat merged commit 004e022 into apache:master Nov 9, 2016

rdhabalia mentioned this pull request Nov 19, 2016

Intermittent test failure SimpleProducerConsumerTest.testUnackBlockRedeliverMessages #93

Closed

rdhabalia deleted the lookup branch January 23, 2017 22:04

jai1 mentioned this pull request Apr 4, 2017

Create HTTP Lookup for CPP Client #307

Closed

merlimat referenced this pull request in merlimat/pulsar Jul 7, 2017

Travis CI setup for website generation (work in progress) (#5)

6181aed

* add protobuf.json to git * make protobuf doc generation static

sijie referenced this pull request in sijie/pulsar Mar 4, 2018

Only test pulsar-functions (#5)

468c6e1

Anonymitaet mentioned this pull request Nov 29, 2019

[Issue 5562][docs]Modify schema compatibility check doc #5757

Merged

liujiejie mentioned this pull request Mar 6, 2020

create the producer will core in c++ #6497

Closed

firefeifei mentioned this pull request Jun 21, 2020

2.6.0 cpp client crash #7327

Closed

klwilson227 mentioned this pull request Aug 19, 2020

pulsar-client-cpp crashes consumer due to thread safty #7851

Closed

hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020

Fixed producer.Flush() (apache#5)

d6a6187

LutzWeischerFujitsu mentioned this pull request Mar 17, 2021

test fails on AArch64, Fedora 33 #9936

Closed

dlg99 referenced this pull request in dlg99/pulsar Jul 20, 2021

Add stripNulls setting to avoid null in Elasticsearch (#5)

7ba63e3

xiaotongwang1 mentioned this pull request Aug 4, 2021

Pulsar 2.7.0+ KOP 2.7.2.x getPartitionedTopicMetadata timeout #11532

Closed

Anonymitaet mentioned this pull request Oct 18, 2021

[website][upgrade]feat: docs migration - version-2.7.2 Pulsar IO #12394

Merged

1 task

iTigeroar mentioned this pull request Nov 19, 2021

c++ client producer 异步发送崩溃 #12887

Closed

paladin-dalao mentioned this pull request Jan 13, 2022

cpp client coredump #13735

Closed

zhcn mentioned this pull request Aug 25, 2022

[Bug] cpp client segmentation fault #17279

Closed

2 tasks

ZHr-UChiHa mentioned this pull request Jun 3, 2023

[Bug] python pulsar client create producer thread suspend because of deadlock apache/pulsar-client-python#129

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Topic lookup using Pulsar binary protocol #5

Support Topic lookup using Pulsar binary protocol #5

rdhabalia commented Sep 10, 2016 •

edited by merlimat

Loading

yahoocla commented Sep 10, 2016

merlimat Sep 10, 2016 •

edited

Loading

rdhabalia Sep 10, 2016

merlimat Sep 12, 2016

rdhabalia Sep 12, 2016

sboobna Sep 15, 2016

rdhabalia Sep 18, 2016

sboobna Oct 3, 2016

sboobna Sep 15, 2016

rdhabalia Sep 19, 2016

sboobna Sep 15, 2016

rdhabalia Sep 18, 2016

sboobna Sep 15, 2016

rdhabalia Sep 19, 2016

sboobna Sep 15, 2016

rdhabalia Sep 19, 2016

sboobna Sep 15, 2016

sboobna Sep 15, 2016

rdhabalia Sep 19, 2016

sboobna Sep 15, 2016

rdhabalia Sep 19, 2016

rdhabalia commented Oct 31, 2016

merlimat Oct 28, 2016

merlimat Oct 28, 2016

rdhabalia Oct 31, 2016

merlimat Oct 28, 2016

merlimat Oct 28, 2016

merlimat commented Nov 9, 2016

merlimat left a comment

Support Topic lookup using Pulsar binary protocol #5

Support Topic lookup using Pulsar binary protocol #5

Conversation

rdhabalia commented Sep 10, 2016 • edited by merlimat Loading

Motivation

Modifications

Result

yahoocla commented Sep 10, 2016

merlimat Sep 10, 2016 • edited Loading

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

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 commented Oct 31, 2016

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 Nov 9, 2016

merlimat left a comment

Choose a reason for hiding this comment

rdhabalia commented Sep 10, 2016 •

edited by merlimat

Loading

merlimat Sep 10, 2016 •

edited

Loading