You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/18 00:57:37 UTC
[GitHub] [pulsar] heesung-sn opened a new pull request, #18079: [improve][load-balance][pip-192] Added ServiceUnitStateChannel interface
heesung-sn opened a new pull request, #18079:
URL: https://github.com/apache/pulsar/pull/18079
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
Fixes #xyz
<!-- or this PR is one task of an issue -->
Master Issue: https://github.com/apache/pulsar/issues/16691
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
We will start raising PRs to implement https://github.com/apache/pulsar/issues/16691
### Modifications
For the pip-192 project, this PR
- defines the classes under `org.apache.pulsar.broker.loadbalance.extensible` package.
- defines this ServiceUnitStateChannel public interface and its expected behaviors.
- defines ServiceUnitState and ServiceUnitStateData data classes.
- defines Split and Unload data classes.
<!-- Describe the modifications you've done. -->
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- *Added unit tests for the new classes.
### Does this pull request potentially affect one of the following parts:
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
We will have separate PRs to update the Doc later.
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #18079: [improve][load-balance][pip-192] Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r997639325
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,99 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │not-owned │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
+
+ Owned, // the ownership assignment is complete (terminal state)
+
+ Assigned, // the ownership is assigned(but the assigned broker has not taken the ownership yet)
+
+ Released, // the ownership is released (e.g. the topic connections are closed)
+
+ Splitting; // the service unit(e.g. bundle) is in the process of splitting.
+ private static Map<ServiceUnitState, Set<ServiceUnitState>> validTransitions = new HashMap<>() {{
+ put(null, new HashSet<>() {{
Review Comment:
Shall we define a state `Free`/`NotOwned` over `null`? Then we can avoid handling business `null`s.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitStateData.java:
##########
@@ -0,0 +1,50 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+import lombok.ToString;
+
+/**
+ * Defines data for the service unit state changes.
+ * This data will be broadcast in ServiceUnitStateChannel.
+ */
+@Data
+@ToString
+@NoArgsConstructor
+@AllArgsConstructor
+public class ServiceUnitStateData {
+ private ServiceUnitState state;
+ private String broker;
+ private String sourceBroker;
+ private long timestamp;
Review Comment:
1. `@Data` already infers `@ToString`.
2. Since the broker JDK version is upgraded to 17, do you think of using [record class](https://docs.oracle.com/en/java/javase/17/language/records.html) to define data models?
3. Are these field intended to be mutable?
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitStateDataTest.java:
##########
@@ -0,0 +1,46 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import static org.apache.pulsar.broker.loadbalance.extensible.channel.ServiceUnitState.Assigned;
+import static org.apache.pulsar.broker.loadbalance.extensible.channel.ServiceUnitState.Owned;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker")
+public class ServiceUnitStateDataTest {
+
+ @Test
+ public void testConstructors() {
+ ServiceUnitStateData data1 = new ServiceUnitStateData(Owned, "A");
+ assertEquals(data1.getState(), Owned);
+ assertEquals(data1.getBroker(), "A");
+ assertNull(data1.getSourceBroker());
+ assertTrue(data1.getTimestamp() > 0);
Review Comment:
ditto other assertions.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,99 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │not-owned │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
+
+ Owned, // the ownership assignment is complete (terminal state)
+
+ Assigned, // the ownership is assigned(but the assigned broker has not taken the ownership yet)
+
+ Released, // the ownership is released (e.g. the topic connections are closed)
+
+ Splitting; // the service unit(e.g. bundle) is in the process of splitting.
+ private static Map<ServiceUnitState, Set<ServiceUnitState>> validTransitions = new HashMap<>() {{
+ put(null, new HashSet<>() {{
Review Comment:
I'd prefer immutable sets over `HashSet` here. For example, `Set.of(...)`.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitStateDataTest.java:
##########
@@ -0,0 +1,46 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import static org.apache.pulsar.broker.loadbalance.extensible.channel.ServiceUnitState.Assigned;
+import static org.apache.pulsar.broker.loadbalance.extensible.channel.ServiceUnitState.Owned;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker")
+public class ServiceUnitStateDataTest {
+
+ @Test
+ public void testConstructors() {
+ ServiceUnitStateData data1 = new ServiceUnitStateData(Owned, "A");
+ assertEquals(data1.getState(), Owned);
+ assertEquals(data1.getBroker(), "A");
+ assertNull(data1.getSourceBroker());
+ assertTrue(data1.getTimestamp() > 0);
Review Comment:
You can use `assertj`'s assertion for better error message:
```java
import static org.assertj.core.api.Assertions.assertThat;
assertThat(data1.getTimestamp()).isGreaterThan(0);
```
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1019781685
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/models/Unload.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.models;
+
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Defines the information required to unload or transfer a service unit(e.g. bundle).
+ */
+public record Unload(String sourceBroker, String serviceUnit, Optional<String> destBroker) {
Review Comment:
I don't see Record related issues right now, and we could easily switch back to the lombok + pojo if blocked by any. By default, I think it is a good practice to try Record as much as we can.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1018729633
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.channel;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign(transfer) ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign(assignment)
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │ free │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
Review Comment:
Is this `ServiceUnit` the same as `bundle` for now and in the foreseeable future?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/models/Split.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.models;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Defines the information required for a bundle split.
+ */
+public record Split(String bundle, String sourceBroker, Map<String, Optional<String>> splitBundleToDestBroker) {
Review Comment:
It's confusing that we use "bundle" here, but use "serviceUnit" in `Unload`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#issuecomment-1283230019
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#issuecomment-1311156893
@Jason918 @eolivelli,
I updated the code based on your comments. Plz, take a look again.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#issuecomment-1282907421
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18079?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18079](https://codecov.io/gh/apache/pulsar/pull/18079?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d7a4a71) into [master](https://codecov.io/gh/apache/pulsar/commit/6c65ca0d8a80bfaaa4d5869e0cea485f5c94369b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6c65ca0) will **decrease** coverage by `4.24%`.
> The diff coverage is `12.50%`.
[](https://codecov.io/gh/apache/pulsar/pull/18079?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18079 +/- ##
============================================
- Coverage 34.91% 30.67% -4.25%
+ Complexity 5707 4016 -1691
============================================
Files 607 397 -210
Lines 53396 43445 -9951
Branches 5712 4463 -1249
============================================
- Hits 18644 13327 -5317
+ Misses 32119 28142 -3977
+ Partials 2633 1976 -657
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `30.67% <12.50%> (-4.25%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18079?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/pulsar/broker/admin/v2/Namespaces.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9OYW1lc3BhY2VzLmphdmE=) | `7.51% <0.00%> (-0.52%)` | :arrow_down: |
| [...adbalance/extensible/channel/ServiceUnitState.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpYmxlL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...lance/extensible/channel/ServiceUnitStateData.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpYmxlL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZURhdGEuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ar/broker/loadbalance/extensible/models/Split.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpYmxlL21vZGVscy9TcGxpdC5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...r/broker/loadbalance/extensible/models/Unload.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpYmxlL21vZGVscy9VbmxvYWQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [.../service/SystemTopicBasedTopicPoliciesService.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1N5c3RlbVRvcGljQmFzZWRUb3BpY1BvbGljaWVzU2VydmljZS5qYXZh) | `49.68% <0.00%> (-1.91%)` | :arrow_down: |
| [.../pulsar/broker/stats/BrokerOperabilityMetrics.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9Ccm9rZXJPcGVyYWJpbGl0eU1ldHJpY3MuamF2YQ==) | `98.21% <ø> (+5.56%)` | :arrow_up: |
| [...g/apache/pulsar/compaction/CompactedTopicImpl.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbXBhY3Rpb24vQ29tcGFjdGVkVG9waWNJbXBsLmphdmE=) | `10.71% <0.00%> (ø)` | |
| [...broker/delayed/InMemoryDelayedDeliveryTracker.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL0luTWVtb3J5RGVsYXllZERlbGl2ZXJ5VHJhY2tlci5qYXZh) | `22.00% <50.00%> (+22.00%)` | :arrow_up: |
| [...apache/pulsar/proxy/server/DirectProxyHandler.java](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLXByb3h5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvcHJveHkvc2VydmVyL0RpcmVjdFByb3h5SGFuZGxlci5qYXZh) | `63.63% <50.00%> (ø)` | |
| ... and [284 more](https://codecov.io/gh/apache/pulsar/pull/18079/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1019781685
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/models/Unload.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.models;
+
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Defines the information required to unload or transfer a service unit(e.g. bundle).
+ */
+public record Unload(String sourceBroker, String serviceUnit, Optional<String> destBroker) {
Review Comment:
I don't see Record related issues right now, and we could easily switch back to the lombok + class if blocked by any. By default, I think it is a good practice to try Record as much as we can.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1019240183
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.channel;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign(transfer) ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign(assignment)
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │ free │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
Review Comment:
I would like to keep open the door to having also a single topic (partition) and not only a "bundle"
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#issuecomment-1282270033
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1019376574
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.channel;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign(transfer) ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign(assignment)
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │ free │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
Review Comment:
I see two conflicting comments here.
The intention is to keep this interface open for other service units(topic, etc). But the initial target service units will be bundles only.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#issuecomment-1309672213
- Updated the package name to `extensions` according to the comment in https://github.com/apache/pulsar/pull/18084#issuecomment-1309449780.
- added the function close() in ServiceUnitStateChannel.
- minor function name updates.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r997751620
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitStateData.java:
##########
@@ -0,0 +1,50 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+import lombok.ToString;
+
+/**
+ * Defines data for the service unit state changes.
+ * This data will be broadcast in ServiceUnitStateChannel.
+ */
+@Data
+@ToString
+@NoArgsConstructor
+@AllArgsConstructor
+public class ServiceUnitStateData {
+ private ServiceUnitState state;
+ private String broker;
+ private String sourceBroker;
+ private long timestamp;
Review Comment:
Updated to `record` classes.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitStateDataTest.java:
##########
@@ -0,0 +1,46 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import static org.apache.pulsar.broker.loadbalance.extensible.channel.ServiceUnitState.Assigned;
+import static org.apache.pulsar.broker.loadbalance.extensible.channel.ServiceUnitState.Owned;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker")
+public class ServiceUnitStateDataTest {
+
+ @Test
+ public void testConstructors() {
+ ServiceUnitStateData data1 = new ServiceUnitStateData(Owned, "A");
+ assertEquals(data1.getState(), Owned);
+ assertEquals(data1.getBroker(), "A");
+ assertNull(data1.getSourceBroker());
+ assertTrue(data1.getTimestamp() > 0);
Review Comment:
Updated.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,99 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │not-owned │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
+
+ Owned, // the ownership assignment is complete (terminal state)
+
+ Assigned, // the ownership is assigned(but the assigned broker has not taken the ownership yet)
+
+ Released, // the ownership is released (e.g. the topic connections are closed)
+
+ Splitting; // the service unit(e.g. bundle) is in the process of splitting.
+ private static Map<ServiceUnitState, Set<ServiceUnitState>> validTransitions = new HashMap<>() {{
+ put(null, new HashSet<>() {{
Review Comment:
Updated.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,99 @@
+/**
+ * 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.pulsar.broker.loadbalance.extensible.channel;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │not-owned │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
+
+ Owned, // the ownership assignment is complete (terminal state)
+
+ Assigned, // the ownership is assigned(but the assigned broker has not taken the ownership yet)
+
+ Released, // the ownership is released (e.g. the topic connections are closed)
+
+ Splitting; // the service unit(e.g. bundle) is in the process of splitting.
+ private static Map<ServiceUnitState, Set<ServiceUnitState>> validTransitions = new HashMap<>() {{
+ put(null, new HashSet<>() {{
Review Comment:
Updated.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#issuecomment-1319498552
> @Jason918 can you take a look again by any chance? We need to merge this first to raise next PRs.
Sorry for the late reply. This PR LGTM. :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #18079:
URL: https://github.com/apache/pulsar/pull/18079
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1019376139
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/models/Unload.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.models;
+
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Defines the information required to unload or transfer a service unit(e.g. bundle).
+ */
+public record Unload(String sourceBroker, String serviceUnit, Optional<String> destBroker) {
Review Comment:
AFAIK, moving forward(since 2.11), Pulsar has started using java 17.
This new load balancer will be available for the new versions of Pulsar (I assume 2.12 >).
The `Unload` and `Split` won't 'be json-serialized(don't get published to topics). These are wrapper objects for internal function calls.
I added the `jsonWriteAndReadTest` UT for `ServiceUnitStateDataTest` for this serialization test.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/models/Split.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.models;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Defines the information required for a bundle split.
+ */
+public record Split(String bundle, String sourceBroker, Map<String, Optional<String>> splitBundleToDestBroker) {
Review Comment:
updated.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1020033394
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.channel;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign(transfer) ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign(assignment)
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │ free │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
Review Comment:
> The intention is to keep this interface open for other service units(topic, etc). But the initial target service units will be bundles only.
Does support for other "service units" included in PIP-192?
Both is fine for me, as long as we don't mixing them up. If we stick to service unit, so there won't be any 'bundle' appear in load balance module unless it's some specific implementation, right?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#issuecomment-1312792590
@Jason918 can you take a look at again by any chance? We need to merge this first to raise next PRs.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1020297171
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitState.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.channel;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Defines the possible states for service units.
+ *
+ * The following diagram defines the valid state changes
+ *
+ * ┌───────────┐
+ * ┌──────────┤ released │◄────────┐
+ * │own └───────────┘ │release
+ * │ │
+ * │ │
+ * ▼ │
+ * ┌────────┐ assign(transfer) ┌─────┴────┐
+ * │ ├───────────────────►│ │
+ * │ owned │ │ assigned │
+ * │ │◄───────────────────┤ │
+ * └──┬─────┤ own └──────────┘
+ * │ ▲ │ ▲
+ * │ │ │ │
+ * │ │ └──────────────┐ │
+ * │ │ │ │
+ * │ │ unload │ │ assign(assignment)
+ * split │ │ │ │
+ * │ │ │ │
+ * │ │ create(child) │ │
+ * │ │ │ │
+ * ▼ │ │ │
+ * ┌─────┴─────┐ └─────►┌───┴──────┐
+ * │ │ │ │
+ * │ splitting ├────────────────► │ free │
+ * │ │ discard(parent)│ │
+ * └───────────┘ └──────────┘
+ */
+public enum ServiceUnitState {
Review Comment:
For pip-192, no, it will be bundle only.
Sure, we can stick to "serviceUnit" for var and func names.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #18079: [improve][broker] PIP-192 Added ServiceUnitStateChannel interface
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #18079:
URL: https://github.com/apache/pulsar/pull/18079#discussion_r1019245081
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/models/Unload.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.models;
+
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Defines the information required to unload or transfer a service unit(e.g. bundle).
+ */
+public record Unload(String sourceBroker, String serviceUnit, Optional<String> destBroker) {
Review Comment:
I am not sure that using "record" is the best choice here.
IIUC it is not possible to add new components to a record without breaking compatibility.
Can you please confirm that this class (and other similar classes) won't be serialised ?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/models/Split.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.pulsar.broker.loadbalance.extensions.models;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Defines the information required for a bundle split.
+ */
+public record Split(String bundle, String sourceBroker, Map<String, Optional<String>> splitBundleToDestBroker) {
Review Comment:
+1 to serviceUnit
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org