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/license feature v2 #18518

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from
Open

Feature/license feature v2 #18518

wants to merge 5 commits into from

Conversation

kvahed
Copy link
Contributor

@kvahed kvahed commented Mar 30, 2023

Scope & Purpose

Expand license feature for resources and components

  • 💩 Bugfix
  • 🍕 New feature
  • 🔥 Performance improvement
  • 🔨 Refactoring/simplification

Checklist

  • Tests
    • Regression tests
    • C++ Unit tests
    • integration tests
    • resilience tests
  • 📖 CHANGELOG entry made
  • 📚 documentation written (release notes, API changes, ...)
  • Backports
    • Backport for 3.10: (Please link PR)
    • Backport for 3.9: (Please link PR)
    • Backport for 3.8: (Please link PR)

Related Information

(Please reference tickets / specification / other PRs etc)

  • Docs PR:
  • Enterprise PR:
  • GitHub issue / Jira ticket:
  • Design document:

@cla-bot cla-bot bot added the cla-signed label Mar 30, 2023
@joerg84 joerg84 requested review from neunhoef and gnusi March 30, 2023 15:20
@joerg84
Copy link
Contributor

joerg84 commented Mar 30, 2023

This is the New PR, please ignore old one

@jsteemann jsteemann added this to the devel milestone Apr 3, 2023
Copy link
Collaborator

@maierlars maierlars left a comment

Choose a reason for hiding this comment

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

NumberOfCoresCache and PhysicalMemoryCache are all local to their compilation unit. (also true for enterprise) Just remove the declaration and everything should still work.

Comment on lines +41 to +45
struct NumberOfCoresCache {
NumberOfCoresCache();
std::atomic<std::size_t> cachedValue;
std::atomic<bool> overridden;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class is unimplemented. It is not the same class as NumberOfCoresCache defined in an anonymous namespace in NumberOfCores.cpp. It this declaration used anywhere?

Comment on lines +41 to +45
struct PhysicalMemoryCache {
PhysicalMemoryCache();
std::atomic<uint64_t> cachedValue;
std::atomic<bool> overridden;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Turns out it is used in the enterprise repository. Calling the constructor of this class will result in a linker error in the community branch.

@maierlars maierlars self-requested a review April 5, 2023 13:12
Copy link
Member

@neunhoef neunhoef left a comment

Choose a reason for hiding this comment

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

I think the determination of the database path is buggy. This needs to be fixed.

@@ -37,6 +37,7 @@
#include "ProgramOptions/Parameters.h"
#include "ProgramOptions/ProgramOptions.h"
#include "ProgramOptions/Section.h"
#include "RestServer/RestartAction.h"
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessary.

@@ -98,6 +98,23 @@ ArangoGlobalContext::ArangoGlobalContext(int /*argc*/, char* argv[],
#endif
#endif

bool lastOptionName{false};
Copy link
Member

Choose a reason for hiding this comment

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

What is this magic for? What happens if the database directory is not given on the command line? Don't we have a DatabasePathFeature for this?

Copy link
Member

Choose a reason for hiding this comment

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

I also do not understand the logic with lastOptionName.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is buggy.

Copy link
Member

Choose a reason for hiding this comment

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

At the very least it deserves an explanation.

@neunhoef
Copy link
Member

neunhoef commented Apr 5, 2023

I have to say that in particular on the last day before a code freeze it is of special importance that all is done such that reviewers have it easy. Contrary to our agreed best practice, this PR:

  • does not have a proper description
  • does not link a design document
  • does not explain what is or should be achieved
  • the PR template is essentially empty, not even the unnecessary parts are removed
    This alone would be enough reason to ask for changes!

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.

None yet

5 participants