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

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

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

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

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

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

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

Please remove this line

removed comment..

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

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

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

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

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

This deserializer could be made static

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

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

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

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

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

Support Topic lookup using Pulsar binary protocol #5

Support Topic lookup using Pulsar binary protocol #5

Uh oh!

Conversation

rdhabalia commented Sep 10, 2016 • edited by merlimat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Result

Uh oh!

yahoocla commented Sep 10, 2016

Uh oh!

merlimat Sep 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdhabalia commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merlimat commented Nov 9, 2016

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rdhabalia commented Sep 10, 2016 •

edited by merlimat

Loading

merlimat Sep 10, 2016 •

edited

Loading