Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220791012
@PSUdaemon There's not any issues I'm aware of at the moment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitH
Github user PSUdaemon commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220764504
Are there still outstanding issues here or is it possible to merge this now?
---
If your project is set up for it, you can reply to this email and have your
rep
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220702623
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
en
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220376323
.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feat
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r63826235
--- Diff: lib/ts/Diags.cc ---
@@ -625,11 +625,14 @@ Diags::should_roll_diagslog()
fflush(diags_log->m_fp);
if (diags_log->roll()
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220231207
@chaiman I see what you mean. I'll fix that in a separate ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user chaiman commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r63660164
--- Diff: lib/ts/Diags.cc ---
@@ -642,11 +645,14 @@ Diags::should_roll_diagslog()
if (diags_log->roll()) {
--- End diff --
sam
Github user chaiman commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r63659601
--- Diff: lib/ts/Diags.cc ---
@@ -625,11 +625,14 @@ Diags::should_roll_diagslog()
fflush(diags_log->m_fp);
if (diags_log->roll
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219347020
@zwoop done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feat
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219342137
Can you squash the commits together too? We don't want intermediary commits
that are failing (they make git bisect difficult to deal with).
---
If your project is
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219324620
@zwoop I've gone ahead and done that. I don't know how (or if I can) to run
the CI builds so I'll just leave that to you.
---
If your project is set up for it, yo
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219261334
This needs a rebase and new clang-format indentation. It fails with
https://ci.trafficserver.apache.org/view/github/job/Github-Linux/67/
---
If your projec
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219194926
Can one of the admins verify this patch? Only approve PRs which have been
reviewed.
---
If your project is set up for it, you can reply to this email and have your
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219191875
Can one of the admins verify this patch? Only approve PRs which have been
reviewed.
---
If your project is set up for it, you can reply to this email and have your
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219190153
Can one of the admins verify this patch? Only approve PRs which have been
reviewed.
---
If your project is set up for it, you can reply to this email and have your
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219186505
Can one of the admins verify this patch? Only approve PRs which have been
reviewed.
---
If your project is set up for it, you can reply to this email and have your
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218929112
Upgraded git on all the build boxes, so try again [approve ci].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitH
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218905112
FreeBSD v10 build finished successfully. Details on
https://ci.trafficserver.apache.org/job/Github-FreeBSD/17/
---
If your project is set up for it, you can
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218904299
Linux (CentOS7) build failed! Details on
https://ci.trafficserver.apache.org/job/Github-Linux/34/
---
If your project is set up for it, you can reply to thi
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218903748
One more round of builds [approve ci].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your proje
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218782605
Can one of the admins verify this patch? Only approve PRs which have been
reviewed.
---
If your project is set up for it, you can reply to this email and have your
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218614205
Linux (CentOS7) build finished successfully. Details on
https://ci.trafficserver.apache.org/job/Github-Linux/17/
---
If your project is set up for it, you c
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218612393
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feat
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218525646
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your pro
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-210516157
@jpeach Thanks for the comments.
I've addressed the relevant issues you brought up in the force pushed
commits. I've also gone ahead and made a separate ti
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-210501721
I don't see any reason to block writing tests on some proposed framework.
All you really need to write a test is a hook to run it (which we have) and a
way to chec
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59890598
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59889953
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59889285
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
Github user danobi commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59888075
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-210488274
While I think unit tests are in general a good idea, I'm not sure how that
could be done in this case and not in the time frame for committing this to
6.2
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209691924
@jpeach Is there a preferred framework for unit testing? (ie how/where do I
put the tests?)
---
If your project is set up for it, you can reply to this email and
Github user danobi commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59644646
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r5964
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209664725
Yes. We looked at various approaches and decided on this one because there
was already a lock around access to the file descriptor. Locking the update to
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209663459
OK the locking makes sense to me I think. You have to hold the lock because
you want to serialize WRT printing output.
``Diags::rebind_stderr`` and ``Diags
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59632436
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59632366
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59632028
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59631832
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59631163
--- Diff: lib/ts/Diags.cc ---
@@ -853,29 +867,39 @@ Diags::set_stderr_output(const char *_bind_stderr)
if (strcmp(_bind_stderr, "") == 0)
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59629895
--- Diff: lib/ts/Diags.cc ---
@@ -642,10 +642,10 @@ Diags::should_roll_diagslog()
bool ret_val = false;
log_log_trace("should_roll_di
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209628730
Looks good to me. Dan, Jason, and I discussed this at length prior to the
work and this looks in line with our concensus.
---
If your project is set up f
GitHub user danobi opened a pull request:
https://github.com/apache/trafficserver/pull/568
TS-4072 Diagnostic log rolling races
This patch prevents race conditions when a diagnostic log rolls. The patch
is a little longer than I had hoped for because of error handling conditions.
Y
44 matches
Mail list logo
