Percona telemetry #5243
Merged
Percona telemetry #5243
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kamil-holubicki
requested review from
percona-ysorokin,
satya-bodapati,
oleksandr-kachan and
dlenev
| REQUIRES_SERVICE_PLACEHOLDER_AS(component_sys_variable_unregister, | ||
| component_sys_variable_unregister_srv); | ||
|
|
||
| static std::unique_ptr<PerconaTelemetryComponent> percona_telemetry_component; |
There was a problem hiding this comment.
anonymous namespace can be used for a group of static declarations
namespace {
...
} // anonymous namespace
There was a problem hiding this comment.
what is the advantage?
There was a problem hiding this comment.
Just stylistic.
Instead of
static int a;
static int b;
...
you can write
namespace {
int a;
int b;
...
}
Basically, you don't have to put static keyword on each declaration.
There was a problem hiding this comment.
I'm still not fully convinced, but let's do it in modern way as you said :)
- Symbols in unnamed namespaces are exported from the library anyway, but their names are randomly mangled. This can cause the linker to do its job longer.
- Maybe I'm old-school, but when I see
static, I exactly know, what it means in all possible contexts. Having an unnamed namespace it is not instantly clear what the function scope is. - If there is only one static variable/function, wrapping it with an anonymous namespace seems to be too verbose. Using
staticin such a case seems to be violating rules: we should use one or another way of limiting the scope, not mixing to avoid mess. - Looks like C++ committee undepreciated use of
static
kamil-holubicki
force-pushed
the
percona-telemetry
branch
from
afbd3aa to
b567438
Compare
kamil-holubicki
force-pushed
the
percona-telemetry
branch
from
468d4b8 to
f12bca2
Compare
kamil-holubicki
force-pushed
the
percona-telemetry
branch
from
f12bca2 to
0067401
Compare
| REQUIRES_SERVICE_PLACEHOLDER_AS(component_sys_variable_unregister, | ||
| component_sys_variable_unregister_srv); | ||
|
|
||
| static std::unique_ptr<PerconaTelemetryComponent> percona_telemetry_component; |
There was a problem hiding this comment.
Just stylistic.
Instead of
static int a;
static int b;
...
you can write
namespace {
int a;
int b;
...
}
Basically, you don't have to put static keyword on each declaration.
| std::filesystem::path::preferred_separator; | ||
|
|
||
| // Let's keep it as std::string, because there is no way to string + string_vew | ||
| static const std::string TMP_FILE_EXTENSION(".tmp"); |
There was a problem hiding this comment.
could be std::string_view
There was a problem hiding this comment.
| // history_keep_interval | ||
| // 2. This is someone's else file, but it is older than the highest | ||
| // possible history_keep_interval | ||
| if (our_file && (file_timestamp < oldest_timestamp)) { |
There was a problem hiding this comment.
Up to you but a few comments:
- std::time_t is an unspecified type, so it may not be long.
- Even if it is long, this is a SIGNED type which may not be what you expect (as you may have already noticed I don't like signed types :) )
- For instance a difference between 2 timestamps may be negative (in case of clock change / ntp adjustments).
kamil-holubicki
force-pushed
the
percona-telemetry
branch
from
d63fe80 to
2cc495f
Compare
kamil-holubicki
force-pushed
the
percona-telemetry
branch
from
2cc495f to
83723c3
Compare
VarunNagaraju
approved these changes
Jun 14, 2024
TEL-46: MySql telemetry component creates telemetry files with not sufficient r/w privileges for TA Problem: Depending on OS configuration, MySql component can create telemetry files with permissions not enough for TA to read their content (e.g. 600). It is required to have permissions mask at least 644. TA deletes files after processing, but deletion permission is a matter of the directory mask, not the file itself. Solution: Storage component adds needed permissions after file creation.
https://perconadev.atlassian.net/browse/PS-9165 Updated copyright info.
kamil-holubicki
force-pushed
the
percona-telemetry
branch
from
abaa584 to
515468d
Compare
kamil-holubicki
merged commit 2cdd70f
into
percona:release-8.0.37-29
5 of 26 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.