-
Notifications
You must be signed in to change notification settings - Fork 838
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
Feature/dismantle mmfiles #11354
Conversation
There was a problem hiding this 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...
@@ -108,9 +94,7 @@ ClusterEngineType ClusterEngine::engineType() const { | |||
} | |||
TRI_ASSERT(_actualEngine != nullptr); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if
seems superfluous
There was a problem hiding this comment.
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;
}
…odb into feature/dismantle-mmfiles
…ture/validation-web-ui * 'devel' of https://github.com/arangodb/arangodb: Feature/dismantle mmfiles (#11354)
…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) ...
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.
Testing & Verification
This change is already covered by existing tests, such as scripts/unittest all.