shibd commented on code in PR #18084: URL: https://github.com/apache/pulsar/pull/18084#discussion_r1025928362
########## pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistry.java: ########## @@ -0,0 +1,101 @@ +/* + * 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; + +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; +import org.apache.pulsar.broker.PulsarServerException; +import org.apache.pulsar.broker.loadbalance.extensions.data.BrokerLookupData; +import org.apache.pulsar.metadata.api.NotificationType; + +/** + * Responsible for registering the current Broker lookup info to + * the distributed store (e.g. Zookeeper) for broker discovery. + */ +public interface BrokerRegistry extends AutoCloseable { + + /** + * Start broker registry. + */ + void start(); + + /** + * Register local broker to metadata store. + */ + void register(); + + /** + * Unregister the broker. + * + * Same as {@link org.apache.pulsar.broker.loadbalance.ModularLoadManager#disableBroker()} + */ + void unregister() throws PulsarServerException; + + /** + * Get the current broker lookup service address. + * + * @return The service url without the protocol prefix, 'http://'. e.g. broker-xyz:port + */ + String getLookupServiceAddress(); + + /** + * Get available brokers. + * + * @return The brokers service url list. + */ + List<String> getAvailableBrokers(); + + /** + * Async get available brokers. + * + * @return The brokers service url list. + */ + CompletableFuture<List<String>> getAvailableBrokersAsync(); + + /** + * Fetch local-broker data from load-manager broker cache. Review Comment: Where is load-manager broker cache? ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistry.java: ########## @@ -0,0 +1,100 @@ +/* + * 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; + +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; +import org.apache.pulsar.broker.PulsarServerException; +import org.apache.pulsar.broker.loadbalance.extensions.data.BrokerLookupData; +import org.apache.pulsar.metadata.api.NotificationType; + +/** + * Responsible for registering the current Broker lookup info to + * the distributed store (e.g. Zookeeper) for broker discovery. + */ +public interface BrokerRegistry { Review Comment: Seems better to change to `BrokerLookupDataRegistry` or `LocalBrokerDataRegistry(if you agree to change BrokerLooupData to LocalBrokerData)` ? ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/LoadDataStore.java: ########## @@ -0,0 +1,65 @@ +/* + * 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.store; + +import java.io.Closeable; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; + +/** + * The load data store interface. + * + * @param <T> The Load data type. + */ +public interface LoadDataStore<T> extends Closeable { + + /** + * Async push load data to store. + * + * @param key The load data key. (e.g. bundle) Review Comment: What are the elements of a bundle? Should there be a namespace to distinguish? ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/LoadManagerScheduler.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.scheduler; + +import java.io.Closeable; + +/** + * The load manager scheduler. + */ +public interface LoadManagerScheduler extends Closeable { Review Comment: Can you explain what this class schedules? ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLookupData.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.data; + +import java.util.Map; +import java.util.Optional; +import org.apache.pulsar.broker.lookup.LookupResult; +import org.apache.pulsar.policies.data.loadbalancer.AdvertisedListener; +import org.apache.pulsar.policies.data.loadbalancer.ServiceLookupData; + +/** + * Defines the information required to broker lookup. + */ +public record BrokerLookupData (String webServiceUrl, Review Comment: Refer [PIP-192](https://github.com/apache/pulsar/issues/16691), this record should be `LocalBrokerData`? ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/LoadDataStore.java: ########## @@ -0,0 +1,65 @@ +/* + * 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.store; + +import java.io.Closeable; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; + +/** + * The load data store interface. + * + * @param <T> The Load data type. + */ +public interface LoadDataStore<T> extends Closeable { Review Comment: Refer PIP, Should change it to `BrokerLoadDataStore`? ########## pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManager.java: ########## @@ -0,0 +1,73 @@ +/* + * 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; + +import java.io.Closeable; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import org.apache.pulsar.broker.PulsarServerException; +import org.apache.pulsar.broker.PulsarService; +import org.apache.pulsar.broker.lookup.LookupResult; +import org.apache.pulsar.broker.namespace.LookupOptions; +import org.apache.pulsar.broker.namespace.NamespaceService; +import org.apache.pulsar.common.naming.NamespaceBundle; +import org.apache.pulsar.common.naming.ServiceUnitId; + +/** + * Find the appropriate broker for service unit (e.g. bundle) through different load balancer Implementation. + */ +public interface ExtensibleLoadManager extends Closeable { + + /** + * Start the extensible load manager. + * + * 1. Start the broker registry. + * 2. Register self to registry. + * 3. Start the load data store. + * 4. Init the load manager context. + * 5. Start load data reporter. + * 6. Start the namespace unload scheduler. + * 7. Start the namespace split scheduler. + * 8. Listen the broker up or down, so we can split immediately. + */ + void start() throws PulsarServerException; + + /** + * Initialize this load manager using the given pulsar service. + */ + void initialize(PulsarService pulsar); + + /** + * The incoming service unit (e.g. bundle) selects the appropriate broker through strategies. + * + * @param topic The optional topic, some method won't provide topic var in this param + * (e.g. {@link NamespaceService#internalGetWebServiceUrl(NamespaceBundle, LookupOptions)}), + * So the topic is optional. + * @param serviceUnit service unit (e.g. bundle). + * @return Simple resource. + */ + CompletableFuture<Optional<LookupResult>> assign(Optional<ServiceUnitId> topic, ServiceUnitId serviceUnit); Review Comment: Who is to invoke this method? Should the method return `BrokerLookupData`? We should minimize coupling with the current implementation. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
