abdullah alamoudi has posted comments on this change. Change subject: Add a dataset rebalance REST API. ......................................................................
Patch Set 10: (8 comments) Didn't proceed much but initial set of comments for now. will continue tomorrow... https://asterix-gerrit.ics.uci.edu/#/c/1768/10/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RebalanceApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RebalanceApiServlet.java: PS10, Line 2: * : * * 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. : * : */ ? PS10, Line 106: out.flush(); don't flush. Flushing forces the http server to chunk the response. better leave it and when it closes, if the response is small enough. it sends it back in one piece. https://asterix-gerrit.ics.uci.edu/#/c/1768/10/asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/RebalanceUtil.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/RebalanceUtil.java: PS10, Line 1: /* : * : * * 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. : * : */ ? PS10, Line 92: Dataset sourceDataset = getSourceDataset(dataverseName, datasetName, metadataProvider); lock released after getting the dataset. someone could drop it at anytime!! The lock acquired should be kept throughout the rebalance operation. the lock should stop all modifications/index builds while allowing search operation. PS10, Line 138: // Finds the dataset that matches the dataverse name and the dataset name. fix comment PS10, Line 164: NodeGroup ng what does it mean if the node group was found?? what if it doesn't contain the same nodes as the ones in the request? https://asterix-gerrit.ics.uci.edu/#/c/1768/10/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/DatasetIdFactory.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/DatasetIdFactory.java: PS10, Line 39: id.compareAndSet(Integer.MAX_VALUE, startId); throw exception instead if it reached max. otherwise, this could lead to catastrophe PS10, Line 43: public static int generateAlternatingDatasetId(int originalId) { : return originalId ^ 0x80000000; : } this is just * -1 ? if so, why not change it? I don't think it is the most performance critical part. -- To view, visit https://asterix-gerrit.ics.uci.edu/1768 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibda35252031fc4940972f0f19bbf796cadfa53d6 Gerrit-PatchSet: 10 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Yingyi Bu <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
