date:20230105

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

2023-01-05 Thread 丛搏

Hi Yunze,

> It's a good idea to use `ProtocolVersion` to control. But adding a
> negative schema type still looks weird.

negative schema type is a history problem(include `NONE` schema type).
I don't think it is a good implementation, It adds too much
complexity. the broker can control any schema-type behavior. like this
problem, don't create sub command with `AUTO_CONSUME` and bring this
problem. We have written a lot of complicated code to solve this
historical problem, If you add new fields it will be more unacceptable
than negative schema type. We should choose a simpler and more direct
way to deal with this matter instead of making it more complicated

Thanks,
Bo

Yunze Xu  于2023年1月6日周五 13:18写道：
>
> Hi Bo,
>
> >  the old server compatibility can add `ProtocolVersion` to control.
>
> It's a good idea to use `ProtocolVersion` to control. But adding a
> negative schema type still looks weird. You can find the following
> description in SchemaType.java, which was added in
> https://github.com/apache/pulsar/pull/3940:
>
> ```java
> // Schemas that don't have schema info. the value should be negative.
> ```
>
> If you expose the negative schema type in PulsarApi.proto, how could
> you explain to users that the "new" schema type is a negative integer?
> And for developers, the negative schema types should not have the
> schema info, but you create a schema info for it.
>
> Thanks,
> Yunze
>
> On Fri, Jan 6, 2023 at 1:07 PM 丛搏  wrote:
> >
> > > Instead, we can add an optional field into CommandSubscribe to
> > > indicate the schema compatibility check is skipped.
> > > ```protobuf
> > > optional bool check_schema_compatibility = 20 [default = true]
> > > ```
> > `check_ schema_ Compatibility 'contains too many meanings. I think
> > this change will make the code more uncontrollable.
> > I still suggest uploading the `AUTO_CONSUME` type directly. the old
> > server compatibility can add `ProtocolVersion` to control. Adding any
> > other fields in proto or uploading directly ` AUTO_ CONSUME ` type
> > makes no difference. Other modifications may lead to ambiguity.
> >
> > Thanks,
> > Bo
> >
> > SiNan Liu  于2023年1月6日周五 00:17写道：
> > >
> > > I just updated the PIP issue and title, you guys can have a look. 
> > > issue19113
> > > 
> > > I added `check_schema_compatibility` in CommandSubscribe, and I also made
> > > many other changes.
> > >
> > > Yunze Xu  于2023年1月5日周四 14:33写道：
> > >
> > > > It's not related to the schema itself. When an AUTO_CONSUME consumer
> > > > subscribes to a topic, the option tells the broker that it's an
> > > > AUTO_CONSUME consumer so that the broker should not treat it as an
> > > > active consumer when performing schema compatibility check. If there
> > > > is a consumer that also wants to ignore the schema compatibility check
> > > > in future, this option can be reused.
> > > >
> > > > The other important reason is the breaking change by carrying the
> > > > schema info on an AUTO_CONSUMER consumer. (See my explanations in
> > > > GitHub and the mail list) If the consumer serves an old version
> > > > consumer, the schema could be uploaded into the registry and other
> > > > clients would be affected. So we should keep not carrying the schema
> > > > info in CommandSubscribe for an AUTO_CONSUMER consumer.
> > > >
> > > > Thanks,
> > > > Yunze
> > > >
> > > > On Thu, Jan 5, 2023 at 11:55 AM SiNan Liu  
> > > > wrote:
> > > > >
> > > > > I have modified pip issue and title last night. Yunze. You mean that 
> > > > > in
> > > > > PulsarApi.proto, take `optional bool is_auto_consume_schema = 6 
> > > > > [default
> > > > =
> > > > > false]; ` in CommandSubscribe instead of Schema? But shouldn't
> > > > > schema-related stuff be in Schema?
> > > > >
> > > > > Thanks,
> > > > > Sinan
> > > > >
> > > > > Yunze Xu  于 2023年1月5日周四 上午12:31写道：
> > > > >
> > > > > > I found a similar compatibility problem with my closed PR. We should
> > > > > > not set the `Schema` field for AUTO_CONSUME schema. More 
> > > > > > explanations
> > > > > > can be found here [1].
> > > > > >
> > > > > > Instead, we can add an optional field into CommandSubscribe to
> > > > > > indicate the schema compatibility check is skipped.
> > > > > >
> > > > > > ```protobuf
> > > > > > optional bool check_schema_compatibility = 20 [default = true]
> > > > > > ```
> > > > > >
> > > > > > Then we can add a relative parameter here:
> > > > > >
> > > > > > ```java
> > > > > > CompletableFuture addSchemaIfIdleOrCheckCompatible(SchemaData
> > > > > > schema, boolean checkSchemaCompatibility);
> > > > > > ```
> > > > > >
> > > > > >
> > > > > > [1]
> > > > https://github.com/apache/pulsar/pull/17449#issuecomment-1371135700
> > > > > >
> > > > > > Thanks,
> > > > > > Yunze
> > > > > >
> > > > > > On Wed, Jan 4, 2023 at 11:49 PM Yunze Xu  
> > > > > > wrote:
> > > > > > >
> > > > > > > Could you also update the PIP issue? This solution is totally
> > > > > > > different from your

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

2023-01-05 Thread Yunze Xu

Hi Bo,

>  the old server compatibility can add `ProtocolVersion` to control.

It's a good idea to use `ProtocolVersion` to control. But adding a
negative schema type still looks weird. You can find the following
description in SchemaType.java, which was added in
https://github.com/apache/pulsar/pull/3940:

```java
// Schemas that don't have schema info. the value should be negative.
```

If you expose the negative schema type in PulsarApi.proto, how could
you explain to users that the "new" schema type is a negative integer?
And for developers, the negative schema types should not have the
schema info, but you create a schema info for it.

Thanks,
Yunze

On Fri, Jan 6, 2023 at 1:07 PM 丛搏  wrote:
>
> > Instead, we can add an optional field into CommandSubscribe to
> > indicate the schema compatibility check is skipped.
> > ```protobuf
> > optional bool check_schema_compatibility = 20 [default = true]
> > ```
> `check_ schema_ Compatibility 'contains too many meanings. I think
> this change will make the code more uncontrollable.
> I still suggest uploading the `AUTO_CONSUME` type directly. the old
> server compatibility can add `ProtocolVersion` to control. Adding any
> other fields in proto or uploading directly ` AUTO_ CONSUME ` type
> makes no difference. Other modifications may lead to ambiguity.
>
> Thanks,
> Bo
>
> SiNan Liu  于2023年1月6日周五 00:17写道：
> >
> > I just updated the PIP issue and title, you guys can have a look. issue19113
> > 
> > I added `check_schema_compatibility` in CommandSubscribe, and I also made
> > many other changes.
> >
> > Yunze Xu  于2023年1月5日周四 14:33写道：
> >
> > > It's not related to the schema itself. When an AUTO_CONSUME consumer
> > > subscribes to a topic, the option tells the broker that it's an
> > > AUTO_CONSUME consumer so that the broker should not treat it as an
> > > active consumer when performing schema compatibility check. If there
> > > is a consumer that also wants to ignore the schema compatibility check
> > > in future, this option can be reused.
> > >
> > > The other important reason is the breaking change by carrying the
> > > schema info on an AUTO_CONSUMER consumer. (See my explanations in
> > > GitHub and the mail list) If the consumer serves an old version
> > > consumer, the schema could be uploaded into the registry and other
> > > clients would be affected. So we should keep not carrying the schema
> > > info in CommandSubscribe for an AUTO_CONSUMER consumer.
> > >
> > > Thanks,
> > > Yunze
> > >
> > > On Thu, Jan 5, 2023 at 11:55 AM SiNan Liu  wrote:
> > > >
> > > > I have modified pip issue and title last night. Yunze. You mean that in
> > > > PulsarApi.proto, take `optional bool is_auto_consume_schema = 6 [default
> > > =
> > > > false]; ` in CommandSubscribe instead of Schema? But shouldn't
> > > > schema-related stuff be in Schema?
> > > >
> > > > Thanks,
> > > > Sinan
> > > >
> > > > Yunze Xu  于 2023年1月5日周四 上午12:31写道：
> > > >
> > > > > I found a similar compatibility problem with my closed PR. We should
> > > > > not set the `Schema` field for AUTO_CONSUME schema. More explanations
> > > > > can be found here [1].
> > > > >
> > > > > Instead, we can add an optional field into CommandSubscribe to
> > > > > indicate the schema compatibility check is skipped.
> > > > >
> > > > > ```protobuf
> > > > > optional bool check_schema_compatibility = 20 [default = true]
> > > > > ```
> > > > >
> > > > > Then we can add a relative parameter here:
> > > > >
> > > > > ```java
> > > > > CompletableFuture addSchemaIfIdleOrCheckCompatible(SchemaData
> > > > > schema, boolean checkSchemaCompatibility);
> > > > > ```
> > > > >
> > > > >
> > > > > [1]
> > > https://github.com/apache/pulsar/pull/17449#issuecomment-1371135700
> > > > >
> > > > > Thanks,
> > > > > Yunze
> > > > >
> > > > > On Wed, Jan 4, 2023 at 11:49 PM Yunze Xu  wrote:
> > > > > >
> > > > > > Could you also update the PIP issue? This solution is totally
> > > > > > different from your original proposal. Since it still introduces
> > > > > > changes to `PulsarApi.proto`, it also requires a PIP (we can reuse
> > > > > > this one).
> > > > > >
> > > > > > 
> > > > > >
> > > > > > BTW, I tested again about carrying the SchemaInfo in the
> > > > > > CommandSubscribe request. It could break compatibility. Given the
> > > > > > following code run against Pulsar standalone 2.8.4:
> > > > > >
> > > > > > ```java
> > > > > > PulsarClient client =
> > > > > > PulsarClient.builder().serviceUrl(serviceUrl).build();
> > > > > > Consumer consumer =
> > > > > > client.newConsumer(Schema.AUTO_CONSUME())
> > > > > > .topic(topic)
> > > > > > .subscriptionName("sub")
> > > > > > .subscriptionType(SubscriptionType.Shared)
> > > > > > .subscribe();
> > > > > > Producer producer =
> > > > > client.newProducer(Schema.AVRO(User.class))
> > > > > > .topic(topic)
> > > > >

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

2023-01-05 Thread 丛搏

> Instead, we can add an optional field into CommandSubscribe to
> indicate the schema compatibility check is skipped.
> ```protobuf
> optional bool check_schema_compatibility = 20 [default = true]
> ```
`check_ schema_ Compatibility 'contains too many meanings. I think
this change will make the code more uncontrollable.
I still suggest uploading the `AUTO_CONSUME` type directly. the old
server compatibility can add `ProtocolVersion` to control. Adding any
other fields in proto or uploading directly ` AUTO_ CONSUME ` type
makes no difference. Other modifications may lead to ambiguity.

Thanks,
Bo

SiNan Liu  于2023年1月6日周五 00:17写道：
>
> I just updated the PIP issue and title, you guys can have a look. issue19113
> 
> I added `check_schema_compatibility` in CommandSubscribe, and I also made
> many other changes.
>
> Yunze Xu  于2023年1月5日周四 14:33写道：
>
> > It's not related to the schema itself. When an AUTO_CONSUME consumer
> > subscribes to a topic, the option tells the broker that it's an
> > AUTO_CONSUME consumer so that the broker should not treat it as an
> > active consumer when performing schema compatibility check. If there
> > is a consumer that also wants to ignore the schema compatibility check
> > in future, this option can be reused.
> >
> > The other important reason is the breaking change by carrying the
> > schema info on an AUTO_CONSUMER consumer. (See my explanations in
> > GitHub and the mail list) If the consumer serves an old version
> > consumer, the schema could be uploaded into the registry and other
> > clients would be affected. So we should keep not carrying the schema
> > info in CommandSubscribe for an AUTO_CONSUMER consumer.
> >
> > Thanks,
> > Yunze
> >
> > On Thu, Jan 5, 2023 at 11:55 AM SiNan Liu  wrote:
> > >
> > > I have modified pip issue and title last night. Yunze. You mean that in
> > > PulsarApi.proto, take `optional bool is_auto_consume_schema = 6 [default
> > =
> > > false]; ` in CommandSubscribe instead of Schema? But shouldn't
> > > schema-related stuff be in Schema?
> > >
> > > Thanks,
> > > Sinan
> > >
> > > Yunze Xu  于 2023年1月5日周四 上午12:31写道：
> > >
> > > > I found a similar compatibility problem with my closed PR. We should
> > > > not set the `Schema` field for AUTO_CONSUME schema. More explanations
> > > > can be found here [1].
> > > >
> > > > Instead, we can add an optional field into CommandSubscribe to
> > > > indicate the schema compatibility check is skipped.
> > > >
> > > > ```protobuf
> > > > optional bool check_schema_compatibility = 20 [default = true]
> > > > ```
> > > >
> > > > Then we can add a relative parameter here:
> > > >
> > > > ```java
> > > > CompletableFuture addSchemaIfIdleOrCheckCompatible(SchemaData
> > > > schema, boolean checkSchemaCompatibility);
> > > > ```
> > > >
> > > >
> > > > [1]
> > https://github.com/apache/pulsar/pull/17449#issuecomment-1371135700
> > > >
> > > > Thanks,
> > > > Yunze
> > > >
> > > > On Wed, Jan 4, 2023 at 11:49 PM Yunze Xu  wrote:
> > > > >
> > > > > Could you also update the PIP issue? This solution is totally
> > > > > different from your original proposal. Since it still introduces
> > > > > changes to `PulsarApi.proto`, it also requires a PIP (we can reuse
> > > > > this one).
> > > > >
> > > > > 
> > > > >
> > > > > BTW, I tested again about carrying the SchemaInfo in the
> > > > > CommandSubscribe request. It could break compatibility. Given the
> > > > > following code run against Pulsar standalone 2.8.4:
> > > > >
> > > > > ```java
> > > > > PulsarClient client =
> > > > > PulsarClient.builder().serviceUrl(serviceUrl).build();
> > > > > Consumer consumer =
> > > > > client.newConsumer(Schema.AUTO_CONSUME())
> > > > > .topic(topic)
> > > > > .subscriptionName("sub")
> > > > > .subscriptionType(SubscriptionType.Shared)
> > > > > .subscribe();
> > > > > Producer producer =
> > > > client.newProducer(Schema.AVRO(User.class))
> > > > > .topic(topic)
> > > > > .create();
> > > > > ```
> > > > >
> > > > > - If the schema type is 0 in CommandSubscribe, the NONE schema will
> > be
> > > > > persisted and the producer will fail to create due to the schema
> > > > > compatibility check.
> > > > > - If the schema type is -3 (AUTO_CONSUME), it will fail at
> > subscribe()
> > > > > with the following error:
> > > > >
> > > > > ```
> > > > > 23:49:10.978 [pulsar-io-18-13] WARN
> > > > > org.apache.pulsar.broker.service.ServerCnx - [/172.23.160.1:5921]
> > Got
> > > > > exception java.lang.IllegalStateException: Some required fields are
> > > > > missing
> > > > > at
> > > >
> > org.apache.pulsar.common.api.proto.Schema.checkRequiredFields(Schema.java:337)
> > > > > at
> > > > org.apache.pulsar.common.api.proto.Schema.parseFrom(Schema.java:332)
> > > > > at
> > > >
> >

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

2023-01-05 Thread Yunze Xu

You only need to describe what's added to the PulsarApi.proto, i.e.
you don't need to paste all definitions of `CommandSubscribe` in the
proposal. Take PIP-54 [1] for example, it only pastes the new field
`ack_set` and does not paste the whole `MessageIdData` definition.

The implementations section involves too much code and just looks like
an actual PR. Take PIP-194 [2] for example, you should only talk about
the implementations from a high level.

Let's talk back to your current design, when the schema type is
AUTO_CONSUME, you clear the schema in CommandSubscribe. It seems that
adding a SchemaInfo to the AutoConsumeSchema is meaningless.

[1] 
https://github.com/apache/pulsar/wiki/PIP-54:-Support-acknowledgment-at-batch-index-level#changes
[2] https://github.com/apache/pulsar/issues/16757

On Fri, Jan 6, 2023 at 12:17 AM SiNan Liu  wrote:
>
> I just updated the PIP issue and title, you guys can have a look. issue19113
> 
> I added `check_schema_compatibility` in CommandSubscribe, and I also made
> many other changes.
>
> Yunze Xu  于2023年1月5日周四 14:33写道：
>
> > It's not related to the schema itself. When an AUTO_CONSUME consumer
> > subscribes to a topic, the option tells the broker that it's an
> > AUTO_CONSUME consumer so that the broker should not treat it as an
> > active consumer when performing schema compatibility check. If there
> > is a consumer that also wants to ignore the schema compatibility check
> > in future, this option can be reused.
> >
> > The other important reason is the breaking change by carrying the
> > schema info on an AUTO_CONSUMER consumer. (See my explanations in
> > GitHub and the mail list) If the consumer serves an old version
> > consumer, the schema could be uploaded into the registry and other
> > clients would be affected. So we should keep not carrying the schema
> > info in CommandSubscribe for an AUTO_CONSUMER consumer.
> >
> > Thanks,
> > Yunze
> >
> > On Thu, Jan 5, 2023 at 11:55 AM SiNan Liu  wrote:
> > >
> > > I have modified pip issue and title last night. Yunze. You mean that in
> > > PulsarApi.proto, take `optional bool is_auto_consume_schema = 6 [default
> > =
> > > false]; ` in CommandSubscribe instead of Schema? But shouldn't
> > > schema-related stuff be in Schema?
> > >
> > > Thanks,
> > > Sinan
> > >
> > > Yunze Xu  于 2023年1月5日周四 上午12:31写道：
> > >
> > > > I found a similar compatibility problem with my closed PR. We should
> > > > not set the `Schema` field for AUTO_CONSUME schema. More explanations
> > > > can be found here [1].
> > > >
> > > > Instead, we can add an optional field into CommandSubscribe to
> > > > indicate the schema compatibility check is skipped.
> > > >
> > > > ```protobuf
> > > > optional bool check_schema_compatibility = 20 [default = true]
> > > > ```
> > > >
> > > > Then we can add a relative parameter here:
> > > >
> > > > ```java
> > > > CompletableFuture addSchemaIfIdleOrCheckCompatible(SchemaData
> > > > schema, boolean checkSchemaCompatibility);
> > > > ```
> > > >
> > > >
> > > > [1]
> > https://github.com/apache/pulsar/pull/17449#issuecomment-1371135700
> > > >
> > > > Thanks,
> > > > Yunze
> > > >
> > > > On Wed, Jan 4, 2023 at 11:49 PM Yunze Xu  wrote:
> > > > >
> > > > > Could you also update the PIP issue? This solution is totally
> > > > > different from your original proposal. Since it still introduces
> > > > > changes to `PulsarApi.proto`, it also requires a PIP (we can reuse
> > > > > this one).
> > > > >
> > > > > 
> > > > >
> > > > > BTW, I tested again about carrying the SchemaInfo in the
> > > > > CommandSubscribe request. It could break compatibility. Given the
> > > > > following code run against Pulsar standalone 2.8.4:
> > > > >
> > > > > ```java
> > > > > PulsarClient client =
> > > > > PulsarClient.builder().serviceUrl(serviceUrl).build();
> > > > > Consumer consumer =
> > > > > client.newConsumer(Schema.AUTO_CONSUME())
> > > > > .topic(topic)
> > > > > .subscriptionName("sub")
> > > > > .subscriptionType(SubscriptionType.Shared)
> > > > > .subscribe();
> > > > > Producer producer =
> > > > client.newProducer(Schema.AVRO(User.class))
> > > > > .topic(topic)
> > > > > .create();
> > > > > ```
> > > > >
> > > > > - If the schema type is 0 in CommandSubscribe, the NONE schema will
> > be
> > > > > persisted and the producer will fail to create due to the schema
> > > > > compatibility check.
> > > > > - If the schema type is -3 (AUTO_CONSUME), it will fail at
> > subscribe()
> > > > > with the following error:
> > > > >
> > > > > ```
> > > > > 23:49:10.978 [pulsar-io-18-13] WARN
> > > > > org.apache.pulsar.broker.service.ServerCnx - [/172.23.160.1:5921]
> > Got
> > > > > exception java.lang.IllegalStateException: Some required fields are
> > > > > missing
> > > > > at
> > > >
> >

Re: [DISCUSS] PIP-232: Introduce thread monitor to check if thread is blocked for long time.

2023-01-05 Thread Heesung Sohn

Hi,

This is a great idea. This will significantly help to debug production
issues caused by long-waiting threads.

I agree with Lari that we could start with the basic metrics.

Like Lari shared in the reference, we could simply collect the current
thread pool queue size like the following.

((ThreadPoolExecutor) executor).getQueue().size()

Also, we can get thread waited time and blocked time via JVM ThreadInfo.
We probably need to enable/disable this JVM ThreadInfo monitoring(thread
contention monitoring) dynamically since this monitoring can consume more
CPU cycles.

https://docs.oracle.com/en/java/javase/17/docs/api/java.management/java/lang/management/ThreadInfo.html

To be specific, some useful broker thread pool metrics might be

// enabled by default
xyzThreadPoolQueueSize(p50,p99,max)

// disabled by default(enabled by dynamic config)
xyzThreadPoolWaitedTime(p50,p99,max)
xyzThreadPoolWaitedCount(p50,p99,max)
xyzThreadPoolBlockedTime(p50,p99,max)
xyzThreadPoolBlockedCount(p50,p99,max)


Additionally,
https://docs.oracle.com/en/java/javase/17/docs/api/java.management/java/lang/management/ThreadInfo.html#getStackTrace()
ThreadInfo contains stack trace, which can help to narrow down the issue
code. (we can print this info in the broker logs.)

Thanks,
Heesung


On Mon, Jan 2, 2023 at 5:31 AM Lari Hotari  wrote:

> This is an interesting proposal. However, I'd suggest changes to the
> current proposal.
>
> I think that the current proposal is too invasive for the Pulsar code
> base. "Introduce thread monitor to check if thread is blocking for long
> time." seems to mean multiple things.
> When looking at the PR, it seems to be a solution for detecting long
> running tasks. Just FYI, that Bookkeeper has a solution for this in it's
> OrderedExecutor with a setting called enableTaskExecutionStats=true . I'm
> not saying that it would be the preferred way to implement it.
>
> If the goal is to detect actual blocking code that is run with threads
> that should run only non-blocking code, there's a better tool called
> Reactor BlockHound (https://github.com/reactor/BlockHound) for that
> purpose.
> For actual profiling of the code base, Java Flight Recorder and Async
> Profiler are better solutions.
>
> It seems that one part of the problem is that there aren't metrics for the
> thread pools. As an alternative implementation for the proposed PIP-232,
> I'd suggest that basic metrics (backlog / queue size, active thread count,
> number of executed tasks, etc)  are added for the thread pools. For
> example, Micrometer contains a decorator for many thread pool
> implementations,
> https://github.com/micrometer-metrics/micrometer/blob/main/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jvm/ExecutorServiceMetrics.java
> . A similar solution would be very useful in Pulsar for adding the thread
> pool metrics.
>
> Tracking individual tasks requires more resources, and that's why I'd
> suggest adding the basic metrics and making them enabled by default. Some
> more advanced metrics would be useful, such as tracking the thread pool
> queue waiting time. Adding a low overhead thread pool queue waiting time
> could be done with a sampling approach. The benefit of that is that there
> won't be a need to wrap all tasks that are executed. There would be several
> ways to implement the queue waiting time metric.
>
> I assume that "blocking" itself might not be the problem and therefore
> having basic metrics (backlog size, active threads, executed tasks counter,
> failed tasks counter) for the thread pools is more essential. There's a lot
> of good things about the PIP-232 proposal and I believe that iterating on
> the ideas will propose a good outcome.
>
> -Lari
>
> On 2022/12/19 12:17:09 adobewjl wrote:
> > Hello pulsar community,
> > I've opened `PIP-232: Introduce thread monitor to check if thread is
> blocked for long time.` to discuss.
> > For more details, please read the PIP at
> https://github.com/apache/pulsar/issues/18985
> > I'm looking forward to hearing what you think.
> > Also the demo PR link at https://github.com/apache/pulsar/pull/18958
>

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

2023-01-05 Thread SiNan Liu

I just updated the PIP issue and title, you guys can have a look. issue19113

I added `check_schema_compatibility` in CommandSubscribe, and I also made
many other changes.

Yunze Xu  于2023年1月5日周四 14:33写道：

> It's not related to the schema itself. When an AUTO_CONSUME consumer
> subscribes to a topic, the option tells the broker that it's an
> AUTO_CONSUME consumer so that the broker should not treat it as an
> active consumer when performing schema compatibility check. If there
> is a consumer that also wants to ignore the schema compatibility check
> in future, this option can be reused.
>
> The other important reason is the breaking change by carrying the
> schema info on an AUTO_CONSUMER consumer. (See my explanations in
> GitHub and the mail list) If the consumer serves an old version
> consumer, the schema could be uploaded into the registry and other
> clients would be affected. So we should keep not carrying the schema
> info in CommandSubscribe for an AUTO_CONSUMER consumer.
>
> Thanks,
> Yunze
>
> On Thu, Jan 5, 2023 at 11:55 AM SiNan Liu  wrote:
> >
> > I have modified pip issue and title last night. Yunze. You mean that in
> > PulsarApi.proto, take `optional bool is_auto_consume_schema = 6 [default
> =
> > false]; ` in CommandSubscribe instead of Schema? But shouldn't
> > schema-related stuff be in Schema?
> >
> > Thanks,
> > Sinan
> >
> > Yunze Xu  于 2023年1月5日周四 上午12:31写道：
> >
> > > I found a similar compatibility problem with my closed PR. We should
> > > not set the `Schema` field for AUTO_CONSUME schema. More explanations
> > > can be found here [1].
> > >
> > > Instead, we can add an optional field into CommandSubscribe to
> > > indicate the schema compatibility check is skipped.
> > >
> > > ```protobuf
> > > optional bool check_schema_compatibility = 20 [default = true]
> > > ```
> > >
> > > Then we can add a relative parameter here:
> > >
> > > ```java
> > > CompletableFuture addSchemaIfIdleOrCheckCompatible(SchemaData
> > > schema, boolean checkSchemaCompatibility);
> > > ```
> > >
> > >
> > > [1]
> https://github.com/apache/pulsar/pull/17449#issuecomment-1371135700
> > >
> > > Thanks,
> > > Yunze
> > >
> > > On Wed, Jan 4, 2023 at 11:49 PM Yunze Xu  wrote:
> > > >
> > > > Could you also update the PIP issue? This solution is totally
> > > > different from your original proposal. Since it still introduces
> > > > changes to `PulsarApi.proto`, it also requires a PIP (we can reuse
> > > > this one).
> > > >
> > > > 
> > > >
> > > > BTW, I tested again about carrying the SchemaInfo in the
> > > > CommandSubscribe request. It could break compatibility. Given the
> > > > following code run against Pulsar standalone 2.8.4:
> > > >
> > > > ```java
> > > > PulsarClient client =
> > > > PulsarClient.builder().serviceUrl(serviceUrl).build();
> > > > Consumer consumer =
> > > > client.newConsumer(Schema.AUTO_CONSUME())
> > > > .topic(topic)
> > > > .subscriptionName("sub")
> > > > .subscriptionType(SubscriptionType.Shared)
> > > > .subscribe();
> > > > Producer producer =
> > > client.newProducer(Schema.AVRO(User.class))
> > > > .topic(topic)
> > > > .create();
> > > > ```
> > > >
> > > > - If the schema type is 0 in CommandSubscribe, the NONE schema will
> be
> > > > persisted and the producer will fail to create due to the schema
> > > > compatibility check.
> > > > - If the schema type is -3 (AUTO_CONSUME), it will fail at
> subscribe()
> > > > with the following error:
> > > >
> > > > ```
> > > > 23:49:10.978 [pulsar-io-18-13] WARN
> > > > org.apache.pulsar.broker.service.ServerCnx - [/172.23.160.1:5921]
> Got
> > > > exception java.lang.IllegalStateException: Some required fields are
> > > > missing
> > > > at
> > >
> org.apache.pulsar.common.api.proto.Schema.checkRequiredFields(Schema.java:337)
> > > > at
> > > org.apache.pulsar.common.api.proto.Schema.parseFrom(Schema.java:332)
> > > > at
> > >
> org.apache.pulsar.common.api.proto.CommandSubscribe.parseFrom(CommandSubscribe.java:785)
> > > > at
> > >
> org.apache.pulsar.common.api.proto.BaseCommand.parseFrom(BaseCommand.java:2397)
> > > > ```
> > > >
> > > > Thanks,
> > > > Yunze
> > > >
> > > >
> > > > On Wed, Jan 4, 2023 at 10:34 PM SiNan Liu 
> > > wrote:
> > > > >
> > > > > I just implemented add an optional field in the subscribe request
> and
> > > > > compatibility seems to be fine. You guys can have a look at my PR (
> > > > > https://github.com/apache/pulsar/pull/17449).
> > > > >
> > > > > Yunze Xu  于2023年1月4日周三 21:31写道：
> > > > >
> > > > > > > Why can't we upload negative schema types?
> > > > > >
> > > > > > I want to avoid the changes to existing methods like
> > > > > > Commands#getSchemaType, which converts all negative schema types
> to
> > > > > > NONE:
> > > > > >
> > > > > > ```java
> > > > > > private static Schema.Type

Re: [Vote] PIP-231: Add metric for topic load failed

2023-01-05 Thread Max Xu

+1 (non-binding)

Best,
Max Xu


On Tue, Dec 27, 2022 at 4:27 PM Jiuming Tao 
wrote:

> Hello pulsar community,
>
> I'm starting the VOTE for PIP-231: Add metric for topic load failed.
>
> Motivation:
> Currently, we have a `topic_load_times` metric to track how long a topic
> load succeeds.
> But when loading a topic, there may be some chances the topic load failed
> due to MetadataStore or Bookkeeper, and we don't have related metrics to
> track it.
>
> For more details, please read the PIP at
> https://github.com/apache/pulsar/issues/18979
> Discuss thread:
> https://lists.apache.org/thread/h02wxj1yclo1o3r7otf9gp38hs0g03ym
>
> Thanks,
> Tao Jiuming
>

Re: [Vote] PIP-231: Add metric for topic load failed

2023-01-05 Thread 易客萧

+1

Yike Xiao

From: Jiuming Tao 
Sent: Tuesday, December 27, 2022 16:27
To: dev@pulsar.apache.org 
Subject: [Vote] PIP-231: Add metric for topic load failed

Hello pulsar community,

I'm starting the VOTE for PIP-231: Add metric for topic load failed.

Motivation:
Currently, we have a `topic_load_times` metric to track how long a topic
load succeeds.
But when loading a topic, there may be some chances the topic load failed
due to MetadataStore or Bookkeeper, and we don't have related metrics to
track it.

For more details, please read the PIP at
https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fpulsar%2Fissues%2F18979=05%7C01%7C%7C497236d1d79d4866f1ae08dae7e4340e%7C84df9e7fe9f640afb435%7C1%7C0%7C638077264574257071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=1ALR7m0noJoS3hPzqr74Gmc8EEdJPuxln4AYzMKDwN0%3D=0
Discuss thread:
https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.apache.org%2Fthread%2Fh02wxj1yclo1o3r7otf9gp38hs0g03ym=05%7C01%7C%7C497236d1d79d4866f1ae08dae7e4340e%7C84df9e7fe9f640afb435%7C1%7C0%7C638077264574257071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=dXm28QSQww%2BslxIUR1JJnWfnv%2BrgVw08pIslnzLP2ik%3D=0

Thanks,
Tao Jiuming

Re: [Vote] PIP-231: Add metric for topic load failed

2023-01-05 Thread houxiaoyu

+1

Best
Xiaoyu Hou

Yunze Xu  于2023年1月5日周四 18:32写道：

> +1 (binding)
>
> Thanks,
> Yunze
>
> On Thu, Jan 5, 2023 at 5:45 PM Jiuming Tao
>  wrote:
> >
> > bump
> >
> > Jiuming Tao  于2022年12月27日周二 16:27写道：
> >
> > > Hello pulsar community,
> > >
> > > I'm starting the VOTE for PIP-231: Add metric for topic load failed.
> > >
> > > Motivation:
> > > Currently, we have a `topic_load_times` metric to track how long a
> topic
> > > load succeeds.
> > > But when loading a topic, there may be some chances the topic load
> failed
> > > due to MetadataStore or Bookkeeper, and we don't have related metrics
> to
> > > track it.
> > >
> > > For more details, please read the PIP at
> > > https://github.com/apache/pulsar/issues/18979
> > > Discuss thread:
> > > https://lists.apache.org/thread/h02wxj1yclo1o3r7otf9gp38hs0g03ym
> > >
> > > Thanks,
> > > Tao Jiuming
> > >
>

Re: [Vote] PIP-231: Add metric for topic load failed

2023-01-05 Thread Yunze Xu

+1 (binding)

Thanks,
Yunze

On Thu, Jan 5, 2023 at 5:45 PM Jiuming Tao
 wrote:
>
> bump
>
> Jiuming Tao  于2022年12月27日周二 16:27写道：
>
> > Hello pulsar community,
> >
> > I'm starting the VOTE for PIP-231: Add metric for topic load failed.
> >
> > Motivation:
> > Currently, we have a `topic_load_times` metric to track how long a topic
> > load succeeds.
> > But when loading a topic, there may be some chances the topic load failed
> > due to MetadataStore or Bookkeeper, and we don't have related metrics to
> > track it.
> >
> > For more details, please read the PIP at
> > https://github.com/apache/pulsar/issues/18979
> > Discuss thread:
> > https://lists.apache.org/thread/h02wxj1yclo1o3r7otf9gp38hs0g03ym
> >
> > Thanks,
> > Tao Jiuming
> >

Re: [VOTE] Pulsar Node.js Client Release 1.8.0 Candidate 2

2023-01-05 Thread Yunze Xu

Hi Nozomi,

AFAIK, the napi-xxx.tar.gz will be uploaded to SVN so that `npm
install` can download these pre-built binaries directly. I didn't look
deeper into the `npm install` process, it might also download the
pre-built binaries from the temporary download links from the
candidate directory.

Thanks,
Yunze

On Thu, Jan 5, 2023 at 5:55 PM Nozomi Kurihara  wrote:
>
> Hi Zike,
>
> Thank you for managing this release.
>
> > However, doing so also creates some inconvenience for the release
> process. I was wondering if we could put these files(napi-xxx.tar.gz)
> in the npm package file.
>
> I'm sorry I don't get the point of your suggestion yet...
> Could you explain
> * What is the "inconvenience" for the release?
> * Will you upload napi-xxx.tar.gz to "both" npm and SVN? (In my
> understanding, uploading to SVN is necessary according to the Apache
> guideline)
> * What is the benefit of uploading to npm?
>
> Best Regards,
> Nozomi

Re: [VOTE] Pulsar Node.js Client Release 1.8.0 Candidate 2

2023-01-05 Thread Nozomi Kurihara

Hi Zike,

Thank you for managing this release.

> However, doing so also creates some inconvenience for the release
process. I was wondering if we could put these files(napi-xxx.tar.gz)
in the npm package file.

I'm sorry I don't get the point of your suggestion yet...
Could you explain
* What is the "inconvenience" for the release?
* Will you upload napi-xxx.tar.gz to "both" npm and SVN? (In my
understanding, uploading to SVN is necessary according to the Apache
guideline)
* What is the benefit of uploading to npm?

Best Regards,
Nozomi

Re: [Vote] PIP-231: Add metric for topic load failed

2023-01-05 Thread Jiuming Tao

bump

Jiuming Tao  于2022年12月27日周二 16:27写道：

> Hello pulsar community,
>
> I'm starting the VOTE for PIP-231: Add metric for topic load failed.
>
> Motivation:
> Currently, we have a `topic_load_times` metric to track how long a topic
> load succeeds.
> But when loading a topic, there may be some chances the topic load failed
> due to MetadataStore or Bookkeeper, and we don't have related metrics to
> track it.
>
> For more details, please read the PIP at
> https://github.com/apache/pulsar/issues/18979
> Discuss thread:
> https://lists.apache.org/thread/h02wxj1yclo1o3r7otf9gp38hs0g03ym
>
> Thanks,
> Tao Jiuming
>

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

Re: [DISCUSS] PIP-232: Introduce thread monitor to check if thread is blocked for long time.

Re: [DISCUSS] PIP-236: Upload AUTO_CONSUME SchemaType to Broker

Re: [Vote] PIP-231: Add metric for topic load failed

Re: [Vote] PIP-231: Add metric for topic load failed

Re: [Vote] PIP-231: Add metric for topic load failed

Re: [Vote] PIP-231: Add metric for topic load failed

Re: [VOTE] Pulsar Node.js Client Release 1.8.0 Candidate 2

Re: [VOTE] Pulsar Node.js Client Release 1.8.0 Candidate 2

Re: [Vote] PIP-231: Add metric for topic load failed

13 matches

Site Navigation

Mail list logo

Footer information