From: Derrick Stolee <stolee@gmail.com>
To: Josh Steadmon <steadmon@google.com>, git@vger.kernel.org
Subject: Re: [RFC PATCH] trace2: don't overload target directories
Date: Tue, 30 Jul 2019 09:29:46 -0400 [thread overview]
Message-ID: <774bc2e2-2081-9969-f5d8-72231a9f7835@gmail.com> (raw)
In-Reply-To: <99e4a0fe409a236d210d95e54cd03fce61daa291.1564438745.git.steadmon@google.com>
On 7/29/2019 6:20 PM, Josh Steadmon wrote:
> trace2 can write files into a target directory. With heavy usage, this
> directory can fill up with files, causing difficulty for
> trace-processing systems.
>
> When trace2 would write a file to a target directory, first check
> whether or not the directory is overloaded. A directory is overloaded if
> there is a sentinel file declaring an overload, or if the number of
> files exceeds a threshold. If the latter, create a sentinel file to
> speed up later overload checks.
>
> The file count threshold is currently set to 1M files, but this can be
> overridden for testing with GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT.
1 million seems like a LOT, and the environment variable seems to be only
for testing.
* If the variable is only for testing, then it should start with GIT_TEST_
* Are we sure 1 million is the right number? I would imagine even 10,000
starting to be a problem. How would a user adjust this value if they
are having problems before 1,000,000?
> The assumption is that a separate trace-processing system is dealing
> with the generated traces; once it processes and removes the sentinel
> file, it should be safe to generate new trace files again.
This matches the model that you (Google) are using for collecting logs.
I'll trust your expertise here in how backed up these logs become. I
imagine that someone working without a network connection for a long
time would be likely to run into this problem.
[snip]
> +test_expect_success "don't overload target directory" '
> + GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT=100 &&
For testing, does this need to be 100? Could it be 5?
> + export GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT &&
To avoid leakage to other (future) tests, should these be in a subshell?
> + test_when_finished "rm -r trace_target_dir" &&
> + mkdir trace_target_dir &&
> + test_seq $GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT | sed "s#^#trace_target_dir/#" | sort > expected_filenames.txt &&
> + xargs touch < expected_filenames.txt &&
nit: no space between redirection and filename.
> + ls trace_target_dir | sed "s#^#trace_target_dir/#" > first_ls_output.txt &&
> + test_cmp expected_filenames.txt first_ls_output.txt &&
> + GIT_TRACE2="$(pwd)/trace_target_dir" test-tool trace2 001return 0 &&
> + echo "trace_target_dir/git-trace2-overload" >> expected_filenames.txt &&
> + ls trace_target_dir | sed "s#^#trace_target_dir/#" > second_ls_output.txt &&
> + test_cmp expected_filenames.txt second_ls_output.txt
> +'
[snip]
> +/*
> + * Check to make sure we're not overloading the target directory with too many
> + * files. First check for the presence of a sentinel file, then check file
> + * count. If we are overloaded, create the sentinel file if it doesn't already
> + * exist.
> + *
> + * We expect that some trace processing system is gradually collecting files
> + * from the target directory; after it removes the sentinel file we'll start
> + * writing traces again.
> + */
> +static int tr2_dst_overloaded(const char *tgt_prefix)
> +{
> + int file_count = 0, overload_file_count = 0;
> + char *test_threshold_val;
> + DIR *dirp;
> + struct strbuf path = STRBUF_INIT, sentinel_path = STRBUF_INIT;
> + struct stat statbuf;
> +
> + strbuf_addstr(&path, tgt_prefix);
> + if (!is_dir_sep(path.buf[path.len - 1])) {
> + strbuf_addch(&path, '/');
> + }
> +
> + /* check sentinel */
> + strbuf_addstr(&sentinel_path, path.buf);
> + strbuf_addstr(&sentinel_path, OVERLOAD_SENTINEL_NAME);
> + if (!stat(sentinel_path.buf, &statbuf)) {
> + strbuf_release(&path);
> + return 1;
> + }
> +
> + /* check if we're overriding the threshold (e.g., for testing) */
> + test_threshold_val = getenv("GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT");
> + if (test_threshold_val)
> + overload_file_count = atoi(test_threshold_val);
> + if (overload_file_count <= 0)
> + overload_file_count = OVERLOAD_FILE_COUNT;
> +
> +
> + /* check file count */
> + dirp = opendir(path.buf);
> + while (file_count < overload_file_count && dirp && readdir(dirp))
> + file_count++;
> + if (dirp)
> + closedir(dirp);
> +
> + if (file_count >= overload_file_count) {
> + creat(sentinel_path.buf, S_IRUSR | S_IWUSR);
> + /* TODO: Write a target-specific message? */
Perhaps leave the TODO out of the code? I did see it in your commit message.
> + strbuf_release(&path);
> + return 1;
> + }
> +
> + strbuf_release(&path);
> + return 0;
> +}
> +
> static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
> {
> int fd;
> @@ -50,6 +121,16 @@ static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
> strbuf_addstr(&path, sid);
> base_path_len = path.len;
>
> + if (tr2_dst_overloaded(tgt_prefix)) {
> + strbuf_release(&path);
> + if (tr2_dst_want_warning())
> + warning("trace2: not opening %s trace file due to too "
> + "many files in target directory %s",
> + tr2_sysenv_display_name(dst->sysenv_var),
> + tgt_prefix);
> + return 0;
> + }
> +
> for (attempt_count = 0; attempt_count < MAX_AUTO_ATTEMPTS; attempt_count++) {
> if (attempt_count > 0) {
> strbuf_setlen(&path, base_path_len);
>
Overall, this looks correct and the test is very clear. Seems to be a helpful feature!
I only have the nits mentioned above.
Thanks!
-Stolee
next prev parent reply other threads:[~2019-07-30 13:29 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-29 22:20 [RFC PATCH] trace2: don't overload target directories Josh Steadmon
2019-07-30 13:29 ` Derrick Stolee [this message]
2019-07-30 21:52 ` Josh Steadmon
2019-07-30 16:46 ` Jeff Hostetler
2019-07-30 22:01 ` Josh Steadmon
2019-07-30 22:02 ` Josh Steadmon
2019-07-30 18:00 ` Jeff Hostetler
2019-07-30 22:08 ` Josh Steadmon
2019-08-02 22:02 ` [RFC PATCH v2 0/2] " Josh Steadmon
2019-08-02 22:02 ` [RFC PATCH v2 1/2] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-08-02 22:02 ` [RFC PATCH v2 2/2] trace2: don't overload target directories Josh Steadmon
2019-08-05 15:34 ` Jeff Hostetler
2019-08-05 18:17 ` Josh Steadmon
2019-08-05 18:01 ` SZEDER Gábor
2019-08-05 18:09 ` Josh Steadmon
2019-09-14 0:25 ` [RFC PATCH v3 0/3] " Josh Steadmon
2019-09-14 0:25 ` [RFC PATCH v3 1/3] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-09-14 0:25 ` [RFC PATCH v3 2/3] trace2: don't overload target directories Josh Steadmon
2019-09-14 0:26 ` [RFC PATCH v3 3/3] trace2: write overload message to sentinel files Josh Steadmon
2019-09-16 12:07 ` Derrick Stolee
2019-09-16 14:11 ` Jeff Hostetler
2019-09-16 18:20 ` Josh Steadmon
2019-09-19 18:23 ` Jeff Hostetler
2019-09-19 22:47 ` Josh Steadmon
2019-09-20 15:59 ` Jeff Hostetler
2019-09-16 18:07 ` Josh Steadmon
2019-10-03 23:32 ` [PATCH v4 0/4] trace2: don't overload target directories Josh Steadmon
2019-10-03 23:32 ` [PATCH v4 1/4] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-10-03 23:32 ` [PATCH v4 2/4] docs: clarify trace2 version invariants Josh Steadmon
2019-10-03 23:32 ` [PATCH v4 3/4] trace2: don't overload target directories Josh Steadmon
2019-10-04 0:25 ` Junio C Hamano
2019-10-04 21:57 ` Josh Steadmon
2019-10-04 9:12 ` Johannes Schindelin
2019-10-04 22:05 ` Josh Steadmon
2019-10-03 23:32 ` [PATCH v4 4/4] trace2: write overload message to sentinel files Josh Steadmon
2019-10-04 22:08 ` [PATCH v5 0/4] trace2: discard new traces if the target directory contains too many files Josh Steadmon
2019-10-04 22:08 ` [PATCH v5 1/4] docs: mention trace2 target-dir mode in git-config Josh Steadmon
2019-10-04 22:08 ` [PATCH v5 2/4] docs: clarify trace2 version invariants Josh Steadmon
2019-10-04 22:08 ` [PATCH v5 3/4] trace2: discard new traces if target directory has too many files Josh Steadmon
2019-10-04 22:08 ` [PATCH v5 4/4] trace2: write discard message to sentinel files Josh Steadmon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=774bc2e2-2081-9969-f5d8-72231a9f7835@gmail.com \
--to=stolee@gmail.com \
--cc=git@vger.kernel.org \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).