stored: buffer fd messages by sebsura · Pull Request #1539 · bareos/bareos

sebsura · 2023-08-29T11:44:38Z

Thank you for contributing to the Bareos Project!

This pr adds an asynchronous message buffer to the sd. This should reduce the time the file daemon has to wait when sending data to the storage daemon.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General

Is the PR title usable as CHANGELOG entry?
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Check backport line
Required backport PRs have been created

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR

Tests

Decision taken that a test is required (if not, then remove this paragraph)
The choice of the type of test (unit test or systemtest) is reasonable
Testname matches exactly what is being tested
On a fail, output of the test leads quickly to the origin of the fault

arogge

Looks pretty solid. Nevertheless, I have a few remarks.

arogge · 2023-09-25T12:23:27Z

core/src/dird/dird_conf.h

+  // base is filled with a list of "base job expressions",
+  // which are just normal strings, with
+  // Base = <text>
+  // inside the option block.  While they are parsed
+  // both by the director and the file daemon, they are
+  // as far as I can see not used at all.
+  // We should think about removing all references to them.
+  // Maybe this was a predecessor of JobResource->base ?
+  // - Sebastian Sura, 12.05.2023
+  alist<const char*> base;      /**< List of base names (unused) */


if it compiles without this, feel free to remove it.

arogge · 2023-09-25T12:24:38Z

core/src/filed/dir_cmd.cc

-        fo->base.destroy();
-        fo->fstype.destroy();
-        fo->Drivetype.destroy();
+        fo->~findFOPTS();


I would strongly favour std::destroy_at() here.

arogge · 2023-09-25T12:26:11Z

core/src/filed/dir_cmd.cc

+        for (auto& regex : fo->regex) { regfree(&regex); }
+        for (auto& regex : fo->regexdir) { regfree(&regex); }
+        for (auto& regex : fo->regexfile) { regfree(&regex); }


what exactly stops us from moving this (and the following free(fo->size_match);) into the dtor?

arogge · 2023-09-25T12:30:17Z

core/src/filed/fileset.cc

+  if (type == ' ') {
+    vec = &current_opts->regex;
+  } else if (type == 'D') {
+    vec = &current_opts->regexdir;
+  } else if (type == 'F') {
+    vec = &current_opts->regexfile;
+  } else {
+    return state_error;
+  }


that looke like it should have been a switch().

arogge · 2023-09-25T12:33:43Z

core/src/findlib/find.cc


-    fnm_flags = BitIsSet(FO_IGNORECASE, ff->flags) ? FNM_CASEFOLD : 0;
-    fnm_flags |= BitIsSet(FO_ENHANCEDWILD, ff->flags) ? FNM_PATHNAME : 0;
+    bool do_exclude = BitIsSet(FO_EXCLUDE, ff->flags);


Suggested change

bool do_exclude = BitIsSet(FO_EXCLUDE, ff->flags);

const bool do_exclude = BitIsSet(FO_EXCLUDE, ff->flags);

arogge · 2023-09-25T13:37:18Z

core/src/win32/findlib/win32.cc

    /* If there is any Drivetype selection set the default
     * selection to false. */
-    if (fo->Drivetype.size()) { wanted = false; }
+    if (fo->Drivetype.size() > 0) { wanted = false; }


Suggested change

if (fo->Drivetype.size() > 0) { wanted = false; }

if (!fo->Drivetype.empty()) { wanted = false; }

core/src/include/baconfig.h

core/src/stored/append.cc

arogge

Looks good. I have two minor remarks that you can address if (and only if) you need to touch the commits again for some reason.

arogge · 2023-11-03T09:29:49Z

core/src/lib/bsock.h

+  static inline constexpr int DataAvailable = 1;
+  static inline constexpr int Timeout = 0;
+  static inline constexpr int Error = -1;


if I'm not mistaken that inline is redundant, but of course not wrong.

Ill have to check again, but at least there is a difference between static and static inline. Not sure if constexpr makes those the same again.

https://stackoverflow.com/a/54467409 global variables should be inline constexpr to fix odr related issues that might arise. static here is just because they are defined inside the class scope.

Seems like constexpr member variables are always inline. Thanks for the hint!

arogge · 2023-11-03T09:40:00Z

core/src/stored/append.cc

    jcr->sd_impl->dcr->rec->data = rec_data;

-    if (bs->IsError()) {
+    if (auto* error = handler.error()) {


as error is passed to Jmsg2() which isn't type-checked at all, I would feel safer with a const char* instead of an auto*

Makes sense!

This class reads data from the given socket asynchroniously and makes them available in a preparsed manner on the main thread. It is important that the given socket is only used by one thread at a time, as such, if you create a MessageHandler for a certain socket, you need to take care to not use that socket at all while the message handler is alive.

sebsura added the draft label Aug 29, 2023

sebsura force-pushed the dev/ssura/master/parallel-receive branch from 8efafcb to 9c22b8d Compare August 31, 2023 13:55

sebsura force-pushed the dev/ssura/master/parallel-receive branch from 9c22b8d to e8c375f Compare September 14, 2023 07:57

sebsura removed the draft label Sep 14, 2023

pstorz requested a review from arogge September 14, 2023 10:20

pstorz assigned arogge Sep 14, 2023

sebsura force-pushed the dev/ssura/master/parallel-receive branch from e8c375f to d127493 Compare September 22, 2023 10:36

arogge requested changes Sep 26, 2023

View reviewed changes

sebsura force-pushed the dev/ssura/master/parallel-receive branch from d127493 to 402bfac Compare October 9, 2023 07:11

sebsura requested a review from arogge October 24, 2023 04:53

sebsura force-pushed the dev/ssura/master/parallel-receive branch from fd0ab2c to 07b7de2 Compare October 24, 2023 04:57

arogge approved these changes Nov 3, 2023

View reviewed changes

sebsura force-pushed the dev/ssura/master/parallel-receive branch from 07b7de2 to 05d7580 Compare November 6, 2023 06:53

sebsura added 9 commits November 7, 2023 07:41

baconfig: increase default network buffer size

4a0061b

append: add maximum to network buffer

ea33476

append: properly propagate read errors

fe79aba

append: fix assigning -1 to an unsigned value

fb24985

append: use channel

5f9d8e5

baconfig: use inline constexpr

2d9f677

dird-cmd: fix data race

dbdde5e

bsock: add named constants for WaitData(Intr)

ad4fc2b

sebsura force-pushed the dev/ssura/master/parallel-receive branch from 05d7580 to ad4fc2b Compare November 7, 2023 06:43

Update CHANGELOG.md

5f12561

BareosBot merged commit 461c984 into bareos:master Nov 7, 2023

	bool do_exclude = BitIsSet(FO_EXCLUDE, ff->flags);
	const bool do_exclude = BitIsSet(FO_EXCLUDE, ff->flags);

	if (fo->Drivetype.size() > 0) { wanted = false; }
	if (!fo->Drivetype.empty()) { wanted = false; }

Conversation

sebsura commented Aug 29, 2023 • edited by arogge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for contributing to the Bareos Project!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Tests

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sebsura commented Aug 29, 2023 •

edited by arogge

Loading