New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the ability to configure a cluster from a JSON file #713

Merged
merged 2 commits into from Sep 5, 2018

Conversation

Projects
None yet
2 participants
@etschannen
Copy link
Contributor

etschannen commented Aug 17, 2018

No description provided.

@etschannen etschannen requested a review from ajbeamon Aug 17, 2018

helpMap["fileconfigure"] = CommandHelp(
"fileconfigure <FILENAME>",
"change the database configuration from a file",
"Load a JSON document from the provided file, and change the database configuration to match the contents of the JSON document. The format should be the same as the value of the \"configuration\" entry in status JSON without \"excluded_servers\" or \"coordinators_count\". Add \"create\":\"new\" to initialize a new database.");

This comment has been minimized.

@ajbeamon

ajbeamon Aug 29, 2018

Contributor

After having thought about this some, I don't think I like having the 'create new' be part of the file contents. Specifically, I don't have a problem with that being supported, but I would rather see it available as part of the command (i.e. fileconfigure new <filename>). If we don't support it externally to the file, then you'd have this awkward situation where the file specifying the configuration at creation differs from the file specifying the configuration at any other time.

StatusObject configJSON = config.get_obj();

json_spirit::mValue schema;
json_spirit::read_string( configurationSchema.toString(), schema );

This comment has been minimized.

@ajbeamon

ajbeamon Aug 29, 2018

Contributor

Should we assert the validity of the schema?

case ConfigurationResult::CONFLICTING_OPTIONS:
case ConfigurationResult::UNKNOWN_OPTION:
case ConfigurationResult::INCOMPLETE_CONFIGURATION:
printUsage(LiteralStringRef("fileconfigure"));

This comment has been minimized.

@ajbeamon

ajbeamon Aug 29, 2018

Contributor

Is this going to be too vague to help someone identify the problem?

}
}

bool schemaMatch( StatusObject const schema, StatusObject const result, Severity sev, bool printErrors, bool checkCoverage, std::string path, std::string schema_path ) {

This comment has been minimized.

@ajbeamon

ajbeamon Aug 29, 2018

Contributor

It feels a little weird to me to have schemaMatch printing the errors for fdbcli. Maybe you could return error details for fdbcli to print out?

#include "flow/flow.h"
#include "FDBTypes.h"

extern const KeyRef statusSchema;

This comment has been minimized.

@ajbeamon

ajbeamon Aug 29, 2018

Contributor

I think I'd prefer these to not be globals like this, in part because usage of statusSchema in other files doesn't really appear as if the variable is defined as such. Maybe you could make them static members of a class?

"$enum":[
"new"
]},
"log_anti_quorum":0,

This comment has been minimized.

@ajbeamon

ajbeamon Aug 29, 2018

Contributor

Tab/space mismatch

"three_data_hall"
]},
"regions":[{
"datacenters":[{

This comment has been minimized.

@ajbeamon

ajbeamon Aug 29, 2018

Contributor

Should we add some indention here to make this easier to read?

@@ -264,6 +281,19 @@ ACTOR Future<ConfigurationResult::Type> changeConfig( Database cx, std::map<std:
m[initIdKey.toString()] = g_random->randomUniqueID().toString();
if (!isCompleteConfiguration(m))
return ConfigurationResult::INCOMPLETE_CONFIGURATION;
} else {
state Future<DatabaseConfiguration> fConfig = getDatabaseConfiguration(cx);
Void _ = wait( success(fConfig) || delay(1.0) );

This comment has been minimized.

@ajbeamon

ajbeamon Sep 5, 2018

Contributor

Are you including the delay here because there are some cases where it will never be possible to read the configuration without first setting it?

This comment has been minimized.

@etschannen

etschannen Sep 5, 2018

Contributor

If you configure "triple" when there is only a single process in the cluster the database will become unavailable and therefore you will not be able to read the database configuration. To resolve this you need to configure "single", so that operation needs to happen even if the database is down.

@ajbeamon ajbeamon merged commit e37f90f into apple:release-6.0 Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment