ENG-3169: Remove setting num_replicas from client API and harden Crea…

…teTable validation in CatalogManager

Summary:
It used to be that we were still supporting a client side override for num_replicas during CreateTable.
Once we introduced the concept of PlacementInfo, we piggybacked on it to setting num_replicas
inside it, which lead to some hacky code for handing both CreateTable providing PlacementInfo (even
if just to set num_replicas) and the cluster already having some in the config.

To remove the ambiguity, I've removed the API to set num_replicas from YBClient all together and we
will have to add it back when we fully support overrides (#149).

Test Plan: Fixed unit tests. Tested with custom placement, including placement_uuid in `yb-ctl` and `yb-admin`.

Reviewers: mikhail, robert, oleg, bharat

Reviewed By: bharat

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D4591

bin/yb-ctl

@@ -349,11 +349,14 @@ class ClusterControl:
             ClusterControl.add_extra_flags_arguments(subparsers[cmd])
             self.options.is_startup_command = True
 
-    # modify_placement_info if this is a startup command and has a placement info.
     def modify_placement_info(self):
-        if self.options.is_startup_command and len(self.options.placement_info) > 0:
+        """
+        This will use yb-admin to set the cluster config object to the desired placement.
+
+        This assumes you will have called set_master_addresses already!
+        """
+        if len(self.options.placement_info) > 0:
             yb_admin_binary_path = self.options.get_binary_path("yb-admin")
-            self.set_master_addresses(on_create=False, running_only=True)
             cmd = [yb_admin_binary_path, "--master_addresses", self.options.master_addresses,
                    "modify_placement_info", self.options.placement_info_raw,
                    str(self.options.replication_factor)]
@@ -372,7 +375,6 @@ class ClusterControl:
         self.args = self.parser.parse_args()
         self.options.update_options_from_args(self.args)
         self.args.func()
-        self.modify_placement_info()
 
     def get_number_of_servers(self, daemon_type):
         return len(glob.glob("{}/*/{}/yb-data/{}".format(
@@ -622,6 +624,7 @@ class ClusterControl:
 
         self.start_helper({DAEMON_TYPE_MASTER: server_counts,
                            DAEMON_TYPE_TSERVER: server_counts})
+        self.modify_placement_info()
         # self.setup_redis(on_create=True)
         print ("Successfully created a new cluster.")
 
@@ -633,6 +636,7 @@ class ClusterControl:
             self.for_all_daemons(self.start_daemon)
         else:
             self.create()
+        self.modify_placement_info()
 
     # Stops the cluster.
     def stop(self):
@@ -651,13 +655,15 @@ class ClusterControl:
         self.set_master_addresses()
         self.for_all_daemons(self.stop_daemon)
         self.for_all_daemons(self.start_daemon)
+        self.modify_placement_info()
 
     def wipe_restart(self):
         num_servers_map = {DAEMON_TYPE_MASTER: self.get_number_of_servers(DAEMON_TYPE_MASTER),
                            DAEMON_TYPE_TSERVER: self.get_number_of_servers(DAEMON_TYPE_TSERVER)}
         self.set_master_addresses()
         self.destroy()
         self.start_helper(num_servers_map)
+        self.modify_placement_info()
 
     def add_node(self):
         self.set_master_addresses(running_only=True)

java/yb-client/src/main/java/org/yb/client/CreateTableOptions.java

@@ -119,19 +119,6 @@ public CreateTableOptions setRangePartitionColumns(List<String> columns) {
     return this;
   }
 
-  /**
-   * Sets the number of replicas that each tablet will have. If not specified, it uses the
-   * server-side default which is usually 3 unless changed by an administrator.
-   *
-   * @param numReplicas the number of replicas to use
-   * @return this instance
-   */
-  public CreateTableOptions setNumReplicas(int numReplicas) {
-    pb.getReplicationInfoBuilder().getLiveReplicasBuilder().setNumReplicas(
-        numReplicas);
-    return this;
-  }
-
   /**
    * Sets the number of tablets.
    *

java/yb-client/src/test/java/org/yb/client/TestAsyncYBClient.java

@@ -51,8 +51,7 @@ public class TestAsyncYBClient extends BaseYBClientTest {
   @Override
   protected void afterStartingMiniCluster() throws Exception {
     super.afterStartingMiniCluster();
-    // Set to 1 for testDisconnect to always test disconnecting the right server.
-    CreateTableOptions options = new CreateTableOptions().setNumReplicas(1);
+    CreateTableOptions options = new CreateTableOptions();
     table = createTable(TABLE_NAME, basicSchema, options);
   }
 

java/yb-client/src/test/java/org/yb/client/TestYBClient.java

@@ -330,11 +330,11 @@ public void testTableCreateCorrectReplicaConfiguration() throws Exception {
     placementBlocksReadOnly.add(placementBlock0);
 
     Master.PlacementInfoPB livePlacementInfo = 
-        Master.PlacementInfoPB.newBuilder().setNumReplicas(3).addAllPlacementBlocks(placementBlocksLive).
+        Master.PlacementInfoPB.newBuilder().addAllPlacementBlocks(placementBlocksLive).
         setPlacementUuid(ByteString.copyFromUtf8(LIVE_TS)).build();
 
     Master.PlacementInfoPB readOnlyPlacementInfo = 
-        Master.PlacementInfoPB.newBuilder().setNumReplicas(3).addAllPlacementBlocks(placementBlocksReadOnly).
+        Master.PlacementInfoPB.newBuilder().addAllPlacementBlocks(placementBlocksReadOnly).
         setPlacementUuid(ByteString.copyFromUtf8(READ_ONLY_TS)).build();
 
     List<Master.PlacementInfoPB> readOnlyPlacements = Arrays.asList(readOnlyPlacementInfo);
@@ -411,7 +411,7 @@ public void testTableCreateCorrectReplicaConfiguration() throws Exception {
     placementBlocksreadOnlyNew.add(placementBlock0);
 
     Master.PlacementInfoPB readOnlyPlacementInfoNew = 
-        Master.PlacementInfoPB.newBuilder().setNumReplicas(3).
+        Master.PlacementInfoPB.newBuilder().
         addAllPlacementBlocks(placementBlocksreadOnlyNew).
         setPlacementUuid(ByteString.copyFromUtf8(READ_ONLY_NEW_TS)).build();
 
@@ -432,7 +432,7 @@ public void testTableCreateCorrectReplicaConfiguration() throws Exception {
                                                                Arrays.asList(8, 8, 8));
     expectedMap.put(READ_ONLY_NEW_TS, expectedReadOnlyNewTsList);
 
-    assertTrue(syncClient.waitForExpectedReplicaMap(30000, table, expectedMap));
+    assertTrue(syncClient.waitForExpectedReplicaMap(60000, table, expectedMap));
   }
 
   /**
@@ -514,7 +514,7 @@ public void testAffinitizedLeaders() throws Exception {
 
     List<ColumnSchema> columns = new ArrayList<>(hashKeySchema.getColumns());
     Schema newSchema = new Schema(columns);
-    CreateTableOptions tableOptions = new CreateTableOptions().setNumReplicas(3).setNumTablets(8);
+    CreateTableOptions tableOptions = new CreateTableOptions().setNumTablets(8);
     YBTable table = syncClient.createTable(DEFAULT_KEYSPACE_NAME, "AffinitizedLeaders", newSchema, tableOptions);
 
     // Wait for leader load balancing to finish, timing out after 30 seconds. 

src/yb/benchmarks/yb_load_test_tool.cc

@@ -75,8 +75,6 @@ DEFINE_int64(max_noop_errors,
              1000,
              "Maximum number of noop errors. The test is aborted after this number of errors.");
 
-DEFINE_int32(num_replicas, 3, "Replication factor for the load test table");
-
 DEFINE_int32(num_tablets, 16, "Number of tablets to create in the table");
 
 DEFINE_bool(noop_only, false, "Only perform noop requests");
@@ -287,7 +285,6 @@ void CreateRedisTable(const YBTableName &table_name, const shared_ptr<YBClient>
   gscoped_ptr<YBTableCreator> table_creator(client->NewTableCreator());
   Status table_creation_status = table_creator->table_name(table_name)
                                      .num_tablets(FLAGS_num_tablets)
-                                     .num_replicas(FLAGS_num_replicas)
                                      .table_type(yb::client::YBTableType::REDIS_TABLE_TYPE)
                                      .Create();
   if (!table_creation_status.ok()) {
@@ -315,7 +312,6 @@ void CreateYBTable(const YBTableName &table_name, const shared_ptr<YBClient> &cl
       table_creator->table_name(table_name)
           .schema(&schema)
           .split_rows(splits)
-          .num_replicas(FLAGS_num_replicas)
           .table_type(yb::client::YBTableType::YQL_TABLE_TYPE)
           .Create();
   if (!table_creation_status.ok()) {