rahulrane50 commented on code in PR #2249: URL: https://github.com/apache/helix/pull/2249#discussion_r1054988257
########## meta-client/src/main/java/org/apache/helix/metaclient/api/ChildChangeListener.java: ########## @@ -0,0 +1,39 @@ +package org.apache.helix.metaclient.api; + +/* + * 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. + */ + +/** + * Listener interface for children change events on a particular key. It includes new child + * creation, child deletion, child data change. + * TODO: add type for persist listener is removed + * For hierarchy key spaces like zookeeper, it refers to an entry's entire subtree. + * For flat key spaces, it refers to keys that matches `prefix*`. + */ +public interface ChildChangeListener { Review Comment: neat. API doc says "DirectChildChangeListener", either of them should be corrected. ########## meta-client/src/main/java/org/apache/helix/metaclient/api/ConnectStateChangeListener.java: ########## @@ -0,0 +1,39 @@ +package org.apache.helix.metaclient.api; + +/* + * 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. + */ + +public interface ConnectStateChangeListener { + /** + * Called when the connection state has changed. I + * @param prevState previous state before state change event. + * @param currentState client state after state change event. If it is a one time listsner, it is + * possible that the metaclient state changes again + */ + void handleConnectStateChanged(MetaClientInterface.ConnectState prevState, MetaClientInterface.ConnectState currentState) throws Exception; Review Comment: Also we don't have any "handleNewSession" handler so are we going to use same ("handleConnectStateChanged") handler when new connection/session is established? if that's the case then what would be prevState in that case because i don't see any "NOT_CONNECTED" state in ConnectState enum ########## meta-client/src/main/java/org/apache/helix/metaclient/api/AsyncCallback.java: ########## @@ -22,28 +22,58 @@ import java.util.List; /** - * An asynchronous callback is deferred to invoke after an async CRUD operation returns. - * The corresponding callback is registered when async CRUD API is invoked. + * An asynchronous callback is deferred to invoke after an async CRUD operation finish and return. + * The corresponding callback is registered when async CRUD API is invoked. Implementation processes + * the result of each CRUD call. It should check return code and perform accordingly. */ +// TODO: define return code. failure code should map to MetaClient exceptions. public interface AsyncCallback { Review Comment: @xyuanlu are you planning to add something like defaultCallback and CancellableAsyncCallback here in future? Because i can see that we support that right now. ########## meta-client/src/main/java/org/apache/helix/metaclient/api/ChildChangeListener.java: ########## @@ -0,0 +1,39 @@ +package org.apache.helix.metaclient.api; + +/* + * 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. + */ + +/** + * Listener interface for children change events on a particular key. It includes new child + * creation, child deletion, child data change. + * TODO: add type for persist listener is removed + * For hierarchy key spaces like zookeeper, it refers to an entry's entire subtree. + * For flat key spaces, it refers to keys that matches `prefix*`. + */ +public interface ChildChangeListener { + enum ChangeType { + ENTRY_CREATED, // Any child entry created + ENTRY_DELETED, // Any child entry deleted + ENTRY_DATA_CHANGE // Any child entry has value change Review Comment: Neat. API doc also talks about "PERSIST_LISTENER_REMOVED", are we going to add this in future? also API documentation is missing with comments about what does it mean. ########## meta-client/src/main/java/org/apache/helix/metaclient/api/ConnectStateChangeListener.java: ########## @@ -0,0 +1,39 @@ +package org.apache.helix.metaclient.api; + +/* + * 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. + */ + +public interface ConnectStateChangeListener { + /** + * Called when the connection state has changed. I + * @param prevState previous state before state change event. + * @param currentState client state after state change event. If it is a one time listsner, it is + * possible that the metaclient state changes again + */ + void handleConnectStateChanged(MetaClientInterface.ConnectState prevState, MetaClientInterface.ConnectState currentState) throws Exception; Review Comment: Curious, why ConnectState is not defined here like other listener implementations? ########## meta-client/src/main/java/org/apache/helix/metaclient/api/DirectChildChangeListener.java: ########## @@ -0,0 +1,37 @@ +package org.apache.helix.metaclient.api; + +/* + * 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. + */ + +/** + * Listener interface for direct child change event on a particular key. It includes new + * child creation or child deletion. The callback won't differentiate these types. + * For hierarchy key spaces like zookeeper, it refers to an entry's direct child nodes. + * For flat key spaces, it refers to keys that matches `prefix*separator`. + */ +public interface DirectChildChangeListener { Review Comment: Same as above comment. I can't figure out from comments about difference between ChildChangeListener and DirectChildChangeListener. Also API doc needs to be updated with both listeners i guess. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
