8000 Feature/single server smart graph by hkernbach · Pull Request #14821 · arangodb/arangodb · GitHub
[go: up one dir, main page]

Skip to content

Feature/single server smart graph #14821

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 relate 8000 d emails.

Already on GitHub? Sign in to your account

Merged
merged 173 commits into from
Nov 17, 2021
Merged

Conversation

hkernbach
Copy link
Member
@hkernbach hkernbach commented Sep 24, 2021

Scope & Purpose

SingleServer SmartGraph Simulator

Description can be found here: https://github.com/arangodb/enterprise/pull/765

  • 🍕 New feature (requires CHANGELOG entry, feature documentation and release notes)

Backports:

  • No backports required

Related Information

IMPORTANT

IMPORTANT

hkernbach and others added 30 commits September 15, 2021 13:14
…db/arangodb into feature/single-server-smart-graph

� Conflicts:
�	arangod/Graph/GraphManager.cpp
…db/arangodb into feature/single-server-smart-graph
…db/arangodb into feature/single-server-smart-graph
…db/arangodb into feature/single-server-smart-graph
…db/arangodb into feature/single-server-smart-graph
…db/arangodb into feature/single-server-smart-graph
from Collections::create, extracted validation and building a builder
Copy link
Member
@mchacki mchacki left a comment

Choose a reason for hiding this comment

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

Majority of this PR looks good. 👍

I have left a couple of comments.
We need to consider a proper archtictural hammer refactoring of the related pieces of Code, this all looks way to complex for such a "little" change.

(!_distributeShardsLike.empty() && ServerState::instance()->isSingleServer() && isSmartOrSatellite)) {
// We either want to expose _distributeShardsLike if we're either on a Coordinator
// Or we have found a Smart or Satellite collection on a single server instance.
if (translateCids && !ServerState::instance()->isSingleServer()) {
Copy link
Member

Choose a reason for hiding this comment

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

Agree that should not be done on single server.
But the case the cluster enters this code path, will exist on the SingleServer as well.
Is the else path on SingleServer enough to handle this case?

@hkernbach
Copy link
Member Author

NOTE for myself. Current State is green.
Current State: CE(d577117) + EE(13cb98b)

image

Copy link
Member
@mchacki mchacki left a comment

Choose a reason for hiding this comment

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

LGTM

if (followingTermId == 0) {
reqOpts.param(StaticStrings::IsSynchronousReplicationString,
ServerState::instance()->getId());
} else {reqOpts.param(StaticStrings::IsSynchronousReplicationString,
Copy link
Member

Choose a reason for hiding this comment

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

This format looks weird

if (followingTermId == 0) {
reqOpts.param(StaticStrings::IsSynchronousReplicationString,
ServerState::instance()->getId());
} else {reqOpts.param(StaticStrings::IsSynchronousReplicationString,
Copy link
Member

Choose a reason for hiding this comment

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

This format looks weird

@mchacki mchacki merged commit 2e7fe93 into devel Nov 17, 2021
@mchacki mchacki deleted the feature/single-server-smart-graph branch November 17, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0