Merge pull request #2218 from yahoo/toregge/add-factory-method-for-delaying-attribute-aspects

Add factory method for delaying attribute aspect changes in document …

5 files changed, 139 insertions, 69 deletions

diff --git a/searchcore/src/tests/proton/documentdb/documentdbconfig/documentdbconfig_test.cpp b/searchcore/src/tests/proton/documentdb/documentdbconfig/documentdbconfig_test.cpp
index a1f5b606415..cc953423604 100644
--- a/searchcore/src/tests/proton/documentdb/documentdbconfig/documentdbconfig_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/documentdbconfig/documentdbconfig_test.cpp
@@ -7,6 +7,8 @@
 #include <vespa/searchcore/proton/test/documentdb_config_builder.h>
 #include <vespa/vespalib/testkit/testapp.h>
 #include <vespa/config-summarymap.h>
+#include <vespa/document/repo/configbuilder.h>
+#include <vespa/document/repo/documenttyperepo.h>
 
 using namespace document;
 using namespace proton;
@@ -17,9 +19,32 @@ using proton::matching::RankingConstants;
 using std::make_shared;
 using std::shared_ptr;
 using vespa::config::search::core::RankingConstantsConfig;
+using document::config_builder::DocumenttypesConfigBuilderHelper;
+using document::config_builder::Struct;
 
 using ConfigSP = shared_ptr<DocumentDBConfig>;
 
+namespace {
+
+const int32_t doc_type_id = 787121340;
+const vespalib::string type_name = "test";
+const vespalib::string header_name = type_name + ".header";
+const vespalib::string body_name = type_name + ".body";
+
+DocumentTypeRepo::SP
+makeDocTypeRepo(bool hasField)
+{
+    DocumenttypesConfigBuilderHelper builder;
+    Struct body(body_name);
+    if (hasField) {
+        body.addField("my_attribute", DataType::T_INT);
+    }
+    builder.document(doc_type_id, type_name, Struct(header_name), body);
+    return make_shared<DocumentTypeRepo>(builder.config());
+}
+
+}
+
 class MyConfigBuilder {
 private:
     test::DocumentDBConfigBuilder _builder;
@@ -58,13 +83,15 @@ public:
         builder.attribute.resize(1);
         AttributesConfigBuilder::Attribute &attribute = builder.attribute.back();
         attribute.name = "my_attribute";
+        attribute.datatype = AttributesConfigBuilder::Attribute::Datatype::INT32;
         _builder.attributes(make_shared<AttributesConfig>(builder));
         return *this;
     }
     MyConfigBuilder &addSummarymap() {
         SummarymapConfigBuilder builder;
         builder.override.resize(1);
-        builder.override.back().field = "my_summary_field";
+        builder.override.back().field = "my_attribute";
+        builder.override.back().command = "attribute";
         _builder.summarymap(make_shared<SummarymapConfig>(builder));
         return *this;
     }
@@ -113,4 +140,53 @@ TEST_F("require that makeReplayConfig() drops unneeded configs", Fixture)
     EXPECT_TRUE(DDBC::preferOriginalConfig(f.nullCfg).get() == nullptr);
 }
 
+struct DelayAttributeAspectFixture {
+    Schema::SP schema;
+    ConfigSP attrCfg;
+    ConfigSP noAttrCfg;
+    DelayAttributeAspectFixture(bool hasDocField)
+        : schema(make_shared<Schema>()),
+          attrCfg(),
+          noAttrCfg()
+    {
+        attrCfg = MyConfigBuilder(4, schema, makeDocTypeRepo(true)).addAttribute().
+                                                   addRankProfile().
+                                                   addRankingConstant().
+                                                   addImportedField().
+                                                   addSummarymap().
+                                                   build();
+        noAttrCfg = MyConfigBuilder(4, schema, makeDocTypeRepo(hasDocField)).addRankProfile().
+                         addRankingConstant().
+                         addImportedField().
+                         build();
+    }
+
+    void assertDelayedConfig(const DocumentDBConfig &testCfg) {
+        EXPECT_FALSE(noAttrCfg->getAttributesConfig() == testCfg.getAttributesConfig());
+        EXPECT_FALSE(noAttrCfg->getSummarymapConfig() == testCfg.getSummarymapConfig());
+        EXPECT_TRUE(attrCfg->getAttributesConfig() == testCfg.getAttributesConfig());
+        EXPECT_TRUE(attrCfg->getSummarymapConfig() == testCfg.getSummarymapConfig());
+        EXPECT_TRUE(testCfg.getDelayedAttributeAspects());
+    }
+    void assertNotDelayedConfig(const DocumentDBConfig &testCfg) {
+        EXPECT_TRUE(noAttrCfg->getAttributesConfig() == testCfg.getAttributesConfig());
+        EXPECT_TRUE(noAttrCfg->getSummarymapConfig() == testCfg.getSummarymapConfig());
+        EXPECT_FALSE(attrCfg->getAttributesConfig() == testCfg.getAttributesConfig());
+        EXPECT_FALSE(attrCfg->getSummarymapConfig() == testCfg.getSummarymapConfig());
+        EXPECT_FALSE(testCfg.getDelayedAttributeAspects());
+    }
+};
+
+TEST_F("require that makeDelayedAttributeAspectConfig works, field remains when attribute removed", DelayAttributeAspectFixture(true))
+{
+    auto delayedRemove = DocumentDBConfig::makeDelayedAttributeAspectConfig(f.noAttrCfg, *f.attrCfg);
+    TEST_DO(f.assertDelayedConfig(*delayedRemove));
+}
+
+TEST_F("require that makeDelayedAttributeAspectConfig works, field removed with attribute", DelayAttributeAspectFixture(false))
+{
+    auto removed = DocumentDBConfig::makeDelayedAttributeAspectConfig(f.noAttrCfg, *f.attrCfg);
+    TEST_DO(f.assertNotDelayedConfig(*removed));
+}
+
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp
index 8c3977d675f..0a02d00980e 100644
--- a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp
@@ -10,7 +10,6 @@
 #include "lid_space_compaction_handler.h"
 #include "maintenance_jobs_injector.h"
 #include "reconfig_params.h"
-#include "configvalidator.h"
 #include <vespa/searchcommon/common/schemaconfigurer.h>
 #include <vespa/searchcore/proton/attribute/attribute_writer.h>
 #include <vespa/searchcore/proton/attribute/imported_attributes_repo.h>
@@ -391,35 +390,6 @@ DocumentDB::performReconfig(DocumentDBConfig::SP configSnapshot)
 
 
 void
-DocumentDB::handleRejectedConfig(DocumentDBConfig::SP &configSnapshot,
-                                 const configvalidator::Result &cvr,
-                                 const DDBState::ConfigState &cs)
-{
-    _state.setConfigState(cs);
-    if (cs == DDBState::ConfigState::NEED_RESTART) {
-        LOG(warning, "DocumentDB(%s): Cannot apply new config snapshot directly: '%s'."
-            " Search node must be restarted for new config to take effect",
-            _docTypeName.toString().c_str(), cvr.what().c_str());
-    } else {
-        LOG(error, "DocumentDB(%s): Cannot apply new config snapshot, new schema is in conflict"
-            " with old schema or history schema: '%s'."
-            " Feed interface is disabled until old config is redeployed",
-            _docTypeName.toString().c_str(), cvr.what().c_str());
-    }
-    LOG(info, "DocumentDB(%s): Config from config server rejected: %s",
-        _docTypeName.toString().c_str(),
-        (cs == DDBState::ConfigState::NEED_RESTART ? "need restart" : "feed disabled"));
-    DocumentDBConfig::SP oaconfig = _activeConfigSnapshot->
-                                    getOriginalConfig();
-    if (!oaconfig ||
-        _state.getState() != DDBState::State::APPLY_LIVE_CONFIG) {
-        configSnapshot = _activeConfigSnapshot;
-    } else {
-        configSnapshot = oaconfig;
-    }
-}
-
-void
 DocumentDB::applySubDBConfig(const DocumentDBConfig &newConfigSnapshot, SerialNum serialNum, const ReconfigParams &params)
 {
     auto registry = _owner.getDocumentDBReferenceRegistry();
@@ -453,38 +423,19 @@ DocumentDB::applyConfig(DocumentDBConfig::SP configSnapshot,
     }
     ConfigComparisonResult cmpres;
     Schema::SP oldSchema;
-    bool fallbackConfig = false;
     int64_t generation = configSnapshot->getGeneration();
     {
         lock_guard guard(_configMutex);
         assert(_activeConfigSnapshot.get());
-        if (_activeConfigSnapshot.get() == configSnapshot.get() ||
-            *_activeConfigSnapshot == *configSnapshot) {
-            // generation might have changed but config is unchanged.
-            if (_state.getRejectedConfig()) {
-                // Illegal reconfig has been reverted.
-                _state.clearRejectedConfig();
-                LOG(info,
-                    "DocumentDB(%s): Config from config server accepted (reverted config)",
-                    _docTypeName.toString().c_str());
-            }
-        } else {
-            oldSchema = _activeConfigSnapshot->getSchemaSP();
-            configvalidator::Result cvr =
-                ConfigValidator::validate(ConfigValidator::Config
-                                          (*configSnapshot->getSchemaSP(),
-                                           configSnapshot->getAttributesConfig()),
-                                          ConfigValidator::Config
-                                          (*oldSchema, _activeConfigSnapshot->getAttributesConfig()),
-                                          *_historySchema);
-            DDBState::ConfigState cs = _state.calcConfigState(cvr.type());
-            if (DDBState::getRejectedConfig(cs))
-            {
-                handleRejectedConfig(configSnapshot, cvr, cs);
-                fallbackConfig = true;
-            }
-            cmpres = _activeConfigSnapshot->compare(*configSnapshot);
+        if (_state.getState() >= DDBState::State::ONLINE) {
+            configSnapshot = DocumentDBConfig::makeDelayedAttributeAspectConfig(configSnapshot, *_activeConfigSnapshot);
+        }
+        if (configSnapshot->getDelayedAttributeAspects()) {
+            _state.setConfigState(DDBState::ConfigState::NEED_RESTART);
+            LOG(info, "DocumentDB(%s): Delaying attribute aspect changes: need restart",
+                _docTypeName.toString().c_str());
         }
+        cmpres = _activeConfigSnapshot->compare(*configSnapshot);
     }
     const ReconfigParams params(cmpres);
     if (params.shouldSchemaChange()) {
@@ -494,7 +445,6 @@ DocumentDB::applyConfig(DocumentDBConfig::SP configSnapshot,
     bool equalReplayConfig =
         *DocumentDBConfig::makeReplayConfig(configSnapshot) ==
         *DocumentDBConfig::makeReplayConfig(_activeConfigSnapshot);
-    assert(!fallbackConfig || equalReplayConfig);
     bool tlsReplayDone = _feedHandler.getTransactionLogReplayDone();
     if (!equalReplayConfig && tlsReplayDone) {
         sync(_feedHandler.getSerialNum());
@@ -536,9 +486,9 @@ DocumentDB::applyConfig(DocumentDBConfig::SP configSnapshot,
     if (params.shouldIndexManagerChange()) {
         setIndexSchema(*configSnapshot, serialNum);
     }
-    if (!fallbackConfig) { 
+    if (!configSnapshot->getDelayedAttributeAspects()) {
         if (_state.getRejectedConfig()) {
-            LOG(info, "DocumentDB(%s): Rejected config replaced with new config",
+            LOG(info, "DocumentDB(%s): Stopped delaying attribute aspect changes",
                 _docTypeName.toString().c_str());
         }
         _state.clearRejectedConfig();
diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.h b/searchcore/src/vespa/searchcore/proton/server/documentdb.h
index d01ae7618f6..371b1c31c70 100644
--- a/searchcore/src/vespa/searchcore/proton/server/documentdb.h
+++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.h
@@ -54,8 +54,6 @@ class IDocumentDBOwner;
 class MetricsWireService;
 class StatusReport;
 
-namespace configvalidator { class Result; }
-
 /**
  * The document database contains all the necessary structures required per
  * document type. It has an internal single-threaded Executor to process input
@@ -159,10 +157,6 @@ private:
     void performReconfig(DocumentDBConfig::SP configSnapshot);
     void closeSubDBs();
 
-    void
-    handleRejectedConfig(DocumentDBConfig::SP &configSnapshot,
-                         const configvalidator::Result &cvr,
-                         const DDBState::ConfigState &cs);
     void applySubDBConfig(const DocumentDBConfig &newConfigSnapshot, SerialNum serialNum, const ReconfigParams &params);
     void applyConfig(DocumentDBConfig::SP configSnapshot, SerialNum serialNum);
 
diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp
index dc1379ee316..820190d62cb 100644
--- a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp
@@ -9,6 +9,9 @@
 #include <vespa/config-summarymap.h>
 #include <vespa/searchsummary/config/config-juniperrc.h>
 #include <vespa/document/config/config-documenttypes.h>
+#include <vespa/searchcore/proton/attribute/attribute_aspect_delayer.h>
+#include <vespa/searchcore/proton/common/document_type_inspector.h>
+#include <vespa/searchcore/proton/common/indexschema_inspector.h>
 
 using namespace config;
 using namespace vespa::config::search::summary;
@@ -74,7 +77,8 @@ DocumentDBConfig::DocumentDBConfig(
       _schema(schema),
       _maintenance(maintenance),
       _extraConfigs(extraConfigs),
-      _orig()
+      _orig(),
+      _delayedAttributeAspects(false)
 { }
 
 
@@ -97,7 +101,8 @@ DocumentDBConfig(const DocumentDBConfig &cfg)
       _schema(cfg._schema),
       _maintenance(cfg._maintenance),
       _extraConfigs(cfg._extraConfigs),
-      _orig(cfg._orig)
+      _orig(cfg._orig),
+      _delayedAttributeAspects(false)
 { }
 
 DocumentDBConfig::~DocumentDBConfig() { }
@@ -270,4 +275,41 @@ DocumentDBConfig::newFromAttributesConfig(const AttributesConfigSP &attributes)
             _extraConfigs);
 }
 
+DocumentDBConfig::SP
+DocumentDBConfig::makeDelayedAttributeAspectConfig(const SP &newCfg, const DocumentDBConfig &oldCfg)
+{
+    const DocumentDBConfig &n = *newCfg;
+    AttributeAspectDelayer attributeAspectDelayer;
+    DocumentTypeInspector inspector(*oldCfg.getDocumentType(), *n.getDocumentType());
+    IndexschemaInspector oldIndexschemaInspector(oldCfg.getIndexschemaConfig());
+    attributeAspectDelayer.setup(oldCfg.getAttributesConfig(), oldCfg.getSummarymapConfig(),
+                                 n.getAttributesConfig(), n.getSummarymapConfig(),
+                                 oldIndexschemaInspector, inspector);
+    bool delayedAttributeAspects = (n.getAttributesConfig() != *attributeAspectDelayer.getAttributesConfig()) ||
+                                   (n.getSummarymapConfig() != *attributeAspectDelayer.getSummarymapConfig());
+    if (!delayedAttributeAspects) {
+        return newCfg;
+    }
+    auto result = std::make_shared<DocumentDBConfig>
+                  (n._generation,
+                   n._rankProfiles,
+                   n._rankingConstants,
+                   n._indexschema,
+                   attributeAspectDelayer.getAttributesConfig(),
+                   n._summary,
+                   attributeAspectDelayer.getSummarymapConfig(),
+                   n._juniperrc,
+                   n._documenttypes,
+                   n._repo,
+                   n._importedFields,
+                   n._tuneFileDocumentDB,
+                   n._schema,
+                   n._maintenance,
+                   n._configId,
+                   n._docTypeName,
+                   n._extraConfigs);
+    result->_delayedAttributeAspects = true;
+    return result;
+}
+
 } // namespace proton
diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h
index b581655330d..bdbce138fd9 100644
--- a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h
+++ b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h
@@ -93,6 +93,7 @@ private:
     MaintenanceConfigSP            _maintenance;
     config::ConfigSnapshot         _extraConfigs;
     SP _orig;
+    bool                           _delayedAttributeAspects;
 
 
     template <typename T>
@@ -155,6 +156,7 @@ public:
     const search::index::Schema::SP &getSchemaSP() const { return _schema; }
     const MaintenanceConfigSP &getMaintenanceConfigSP() const { return _maintenance; }
     const search::TuneFileDocumentDB::SP &getTuneFileDocumentDBSP() const { return _tuneFileDocumentDB; }
+    bool getDelayedAttributeAspects() const { return _delayedAttributeAspects; }
 
     bool operator==(const DocumentDBConfig &rhs) const;
 
@@ -189,6 +191,12 @@ public:
      * Create modified attributes config.
      */
     SP newFromAttributesConfig(const AttributesConfigSP &attributes) const;
+
+    /**
+     * Create config with delayed attribute aspect changes if they require
+     * reprocessing.
+     */
+    static SP makeDelayedAttributeAspectConfig(const SP &newCfg, const DocumentDBConfig &oldCfg);
 };
 
 } // namespace proton