Skip to content
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

Feature/dismantle mmfiles #11354

Merged
merged 33 commits into from
Apr 2, 2020
Merged

Feature/dismantle mmfiles #11354

merged 33 commits into from
Apr 2, 2020

Conversation

jsteemann
Copy link
Contributor

@jsteemann jsteemann commented Mar 31, 2020

Scope & Purpose

Dismantle MMFiles storage engine in devel.
Enterprise companion PR: https://github.com/arangodb/enterprise/pull/423
Oskar PR: arangodb/oskar#233

Documentation is still to do.

  • Strictly new functionality (i.e. a new feature / new option, no need for porting)
  • The behavior in this PR can be (and was) manually tested (support / qa / customers can test it)
  • The behavior change can be verified via automatic tests

Testing & Verification

This change is already covered by existing tests, such as scripts/unittest all.

@jsteemann
Copy link
Contributor Author

@jsteemann
Copy link
Contributor Author

jsteemann commented Mar 31, 2020

Copy link
Contributor

@joerg84 joerg84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the 50% (i.e., 230 FIles). Not an expert on many of the topics...

arangod/ClusterEngine/ClusterCollection.cpp Show resolved Hide resolved
@@ -108,9 +94,7 @@ ClusterEngineType ClusterEngine::engineType() const {
}
TRI_ASSERT(_actualEngine != nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curiosity, not related to this PR: ClusterEngine::isMock() const. above doesn't check . _actualEngine but accesses name. Maybe add same assert there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClusterEngine::isMock() also returns true when _actualEngine is a nullptr, but ClusterEngine::Mocking is set to true.
Some in ClusterEngine::isMock() we cannot rely on _actualEngine being a non-nullptr.

And both isMock() and isRocksDB() access the name() method of _actualEngine to check which engine is in use, but both only do this if _actualEngine is a non-nullptr. So it should be ok as it is.

if (isMMFiles()) {
return ClusterEngineType::MMFilesEngine;
} else if (isRocksDB()) {
if (isRocksDB()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if seems superfluous

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to

@@ -94,12 +94,8 @@ ClusterEngineType ClusterEngine::engineType() const {
   }
   TRI_ASSERT(_actualEngine != nullptr);
 
-  if (isRocksDB()) {
-    return ClusterEngineType::RocksDBEngine;
-  }
-
-  TRI_ASSERT(false);
-  THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "invalid engine type");
+  TRI_ASSERT(isRocksDB());
+  return ClusterEngineType::RocksDBEngine;
 }
 

arangod/RocksDBEngine/RocksDBCollection.cpp Show resolved Hide resolved
arangod/StorageEngine/HotBackup.cpp Outdated Show resolved Hide resolved
@jsteemann
Copy link
Contributor Author

arangod/ClusterEngine/ClusterCollection.cpp Show resolved Hide resolved
arangod/ClusterEngine/ClusterCollection.cpp Show resolved Hide resolved
arangod/RestServer/DatabaseFeature.cpp Show resolved Hide resolved
arangod/RestServer/DatabaseFeature.cpp Show resolved Hide resolved
arangod/VocBase/Methods/UpgradeTasks.cpp Show resolved Hide resolved
arangod/VocBase/Methods/UpgradeTasks.cpp Show resolved Hide resolved
arangod/RestServer/FlushFeature.cpp Show resolved Hide resolved
@jsteemann
Copy link
Contributor Author

@jsteemann
Copy link
Contributor Author

@jsteemann jsteemann merged commit 4284d2f into devel Apr 2, 2020
@jsteemann jsteemann deleted the feature/dismantle-mmfiles branch April 2, 2020 12:17
ObiWahn added a commit that referenced this pull request Apr 2, 2020
ObiWahn added a commit that referenced this pull request Apr 6, 2020
…ture/utf-8-validation

* 'devel' of https://github.com/arangodb/arangodb: (21 commits)
  Bug fix/headers cleanup (#11391)
  Feature/internal issue #672 (#11370)
  GraphNodes now copy the graph when cloned (#11345)
  fix lame compile error
  Feature/aql subquery execution block impl execute implementation harvesting (#11349)
  remove obsolete recoveryData
  Feature/dismantle mmfiles (#11354)
  Fix an agency supervision bug. (#11356)
  (mostly) restore pre-3.7 API behavior (#11364)
  validation: AQL functions (#11327)
  Check MSVC_VERSION instead of CMAKE_GENERATOR (#11351)
  added sleep time (lousy fix)
  Feature/aql interleave function (#11352)
  cheapify IN lookups on unsorted arrays (#11342)
  Bug fix/fix msvc2019 build (#11052)
  Allow easier removal of validation rules. (#11346)
  upgrade RocksDB (#11308)
  Introduce more type-safe identifiers (#11270)
  Bug fix/schema validation return code (#11341)
  Fix explainer output when restricting collections (#11338)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants