Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105569519
Wireshark decrypts SSL, https://wiki.wireshark.org/SSL.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105570715
Well if you can configure logging on the ATS server, then it seems
reasonable that you would have access to the SSL keys.
---
If your project is set up for it
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/206#issuecomment-107628793
The ``REC_SRC`` constants scan poorly, I think I'd prefer ``REC_SOURCE``.
``REC_SRC_NONE`` should be called ``REC_SOURCE_NULL`` to be consistent with
other n
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/206#issuecomment-107638604
Yes, I had the same conflict about ``RECS`` ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/206#issuecomment-107664507
There's no way to know whether a metric was set by ``traffic_line``. The
best you can do is to say whether a metric was set by a specific API, but
that'
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-108485861
Wait, why isn't page speed integrated into the automake build? We add these
macros to the build globally at configuration time.
---
If your project is set u
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-108487426
That's fine, we have other stuff that only builds when it's dependencies
are available. I'm happy to advise if you want help doing this in autotools
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/213#issuecomment-109071471
I think you need to clean up this branch, removing commits that aren't
intended for upstream. A more descriptive pull request message would also help
us f
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/223#issuecomment-111547708
How was this tested?
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/231#issuecomment-113320883
Can you show the pahole(1) results for before and after?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/232#issuecomment-114935138
I made couple of specific comments on ae455eb.
Some more general comments on 1d38e70 needs license headers, header
guards should use the existing
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-120036935
I think @ushachar is suggesting that this whole feature should be in a
plugin?
---
If your project is set up for it, you can reply to this email and have your
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/245#issuecomment-120039117
I'm strongly in favor of introspecting HostDB, but I don't like extending
HTTPUI. The right way to do this IMHO is to add management APIs so you can
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/245#issuecomment-120151397
Because generating raw HTML in core code is unmaintainable. The right
approach is to extract the data and publish it in the appropriate format for
the consumer
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122596851
But what you are saying there is that the config *might* do what it says if
you have aligned the starts appropriately. I don't think that's g
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/258#issuecomment-123396100
Looks quite reasonable. I think we can improve the commit messages though.
[These](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/262#issuecomment-125250513
I'm not sure that always falling back to a default is desirable. If the
operator specified an empty ```resolv.conf```, then why wouldn't we use
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/262#issuecomment-125251374
Nothing? Use the resolvers in ```records.config```? Assume every name is in
```/etc/hosts```? Never use DNS names?
---
If your project is set up for it, you can
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/262#issuecomment-125252893
That seems reasonable, and WARNing on an empty file also seems reasonable.
Crashing, less so.
---
If your project is set up for it, you can reply to this email
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/271#issuecomment-128482080
Other HTTP servers do send the Content-Length on HEAD requests; I think
that we should do the same.
---
If your project is set up for it, you can reply to this
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/274#issuecomment-129955776
Can you explain why we need this, rather than adding better support for
something like log rotate?
---
If your project is set up for it, you can reply to this
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130113092
I don't really see why ```ip_filter``` should not be able to deal with
webdav methods. It seems reasonable to me that you should be able to filter any
la
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/280#issuecomment-134649058
This branch needs to be cleaned up before it can be merged. Please rebase
the patches so that there is:
- a patch that adds CacheProcessor
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/280#issuecomment-135156363
What I mean is to separate refactoring patches from patches that change
behaviour. In a nice, clean patch series, make the refactoring up front in
independent
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/284#discussion_r38252875
--- Diff: doc/reference/plugins/header_rewrite.en.rst ---
@@ -208,3 +208,44 @@ Examples
rm-header Set-Cookie
counter
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/284#discussion_r38252894
--- Diff: doc/reference/plugins/header_rewrite.en.rst ---
@@ -208,3 +208,44 @@ Examples
rm-header Set-Cookie
counter
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/284#discussion_r38252920
--- Diff: plugins/header_rewrite/conditions.cc ---
@@ -237,13 +236,22 @@ ConditionHeader::append_value(std::string &s, const
Resources
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/286#issuecomment-136489609
I ran the regression tests on OS X with this patch, and saw some [memory
leaks](http://apaste.info/z1O). the ```test_certlookup``` test (part of ```make
check
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/282#issuecomment-136797726
You don't need to do that, just create the files you want with ```dd```.
---
If your project is set up for it, you can reply to this email and have your
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/295#issuecomment-141333600
This looks reasonable. Could you add a unit test for
```http2_are_frame_flags_valid()```, which should be straightforward now the
masks are correct.
I
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/295#issuecomment-141487839
Merged under TS-3827. Let's restore the flags check under a separate JIRA.
Thanks!
---
If your project is set up for it, you can reply to this email and
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/297#issuecomment-141521724
You should use ```__func__``` in debug messages rather than typing the
function name all the tine :)
---
If your project is set up for it, you can reply to this
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/297#issuecomment-141522930
As a separate ticket, if you just rename the original ```Range``` header to
```@Range```, you can remove the per-transaction data, saving yourself the
copying and
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/283#issuecomment-141525480
@PSUdaemon, @bgaff, @SolidWallOfCode: can you look at this one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/297#issuecomment-141525653
You would still re-add, but renaming means you don't need to do a bunch of
copies. I agree that restoring ```Range``` is the right thing.
---
If your proje
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/299#issuecomment-145315221
Merged. Thanks!
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/300#issuecomment-145649988
I think we need tests for this.
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/304#issuecomment-146751892
How does this happen? There are lots of places that iterate over the
records without holding the read lock. We should make sure that the lock is
held correctly
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/304#issuecomment-146912485
For example, ```RecLookupMatchingRecords``` doesn't lock around
```g_records```, likewise ```g_records``` is not locked anywhere in
```P_RecCore.cc```
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/190#issuecomment-147924237
@bgaff can you please review and merge?
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/272#issuecomment-147929606
*TSHttpTxnApplyLogFormat*
- I think the name could be clearer, maybe TSHttpTxnFormatLogTags() or
TSHttpFormatTags().
- It seems reasonable to format
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/232#issuecomment-147932185
Sorry for dropping the ball on this one @repodude. If you are coming to the
ATS summit, I'd like to sit with you and get this merged. There's a few styl
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-147933555
I will review this. Hoping to get to it by the weekend of Oct 17th. There
are some style changes to make, but I'm mainly concerned about the integration
wit
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/305#issuecomment-149930096
It will be in 6.0.1, but I don't know whether we have a schedule for that.
---
If your project is set up for it, you can reply to this email and have your
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/298#issuecomment-150393196
@Humbedooh can you please close this?
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-150839902
ink_file_lmtime
- Should be named "ink_file_time".
- I don't see why we need to use ink_timezone() here, or anywhere else for
that mat
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/312#issuecomment-151226907
I merged the second change but not the first. The first looks like a
legitimate error case.
---
If your project is set up for it, you can reply to this email and
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-151318976
I don't think it is that complex. All the machinery for doing this exists,
it just needs to be tied together in a sightly different way. My view is that
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-151591704
The local manager can be used to send a message from ```traffic_server```
up to ```traffic_manager```. Look for ```RecMessageSend```. It's unfortunate
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/317#issuecomment-152620337
```update.config``` is no more, could you please remove it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/321#issuecomment-153798201
I think that we really need to break this feature down into smaller commits
before we can digest and merge it. Right now the only way I can see doing it is
if we
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153949955
How do we end up freeing a partially-initialized ```SpdyClientSession```?
It looks like this does not fix the root cause of the problem.
---
If your project is
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153950616
But the only way this could happen is if the session was released before
```init``` was called, right?
---
If your project is set up for it, you can reply to
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153951633
Yeh that's the way these object usually work. ```SpdyClientSession``` is a
bit different from the other client session objects because last time I cleaned
th
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153956478
Ah I see now. Then probably the right fix is to remove
```~SpdyClientSession()```. I don't think any other proxy-allocated object have
destructors, since
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-154125097
Yup, ```SpdyRequest``` has the same issue. I have no objection to improving
the way ```ClassAllocator``` works in a separate ticket. One thing you could
explore
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-154250586
I agree that this change doesn't make anything worse :) Would you mind
making a followup change to remove the destructors in the SPDY code?
---
If your pr
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/324#issuecomment-154492445
I think that if you just pass ```-R/path/to/foo``` to ```TS_ADDTO()``` it
will do the right thing. I like the addition of the RPATH option. You don't
ne
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/330#issuecomment-155227320
Could you also add a test for this? Otherwise it is very likely to regress
in the future.
---
If your project is set up for it, you can reply to this email and
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/321#issuecomment-156858819
John and I spoke about how we can get this merged. The plan we came up with
is to make a separate pull request that contains just the major strategy
pattern
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/190#issuecomment-157196927
AFAICT this was fixed in https://issues.apache.org/jira/browse/TS-3759.
Thanks @zeb209
---
If your project is set up for it, you can reply to this email and have
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-157497990
- Remove _strlen and _memcpy usage (see TS-4029).
- In ConfigFileGroup(), use ats_strdup() rather than malloc+memcpy.
- In Rollback::createPathStr
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-157786700
> 1. How to clean up the old watched children files correctly?
If you have a tree of ```Rollback``` objects, then any object in the tree
that is upda
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-158530197
Thanks, I'll review again next week.
I don't think I understand what you mean about the marshaling APIs being
unsuitable. Here's how you
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/343#issuecomment-159090606
> I thought I saw a comment from James but it doesn't seem to be there now.
I think I made them on the JIRA.
> 1) I'm open to us
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159418105
```ContFlags.{cc,h}``` appear to be missing. I'm with @ushachar on this; I
think the capability is useful but I'm concerned about having the policy in
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-159418922
@zizhong Great! I don't see the new patch here though?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitH
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/343#issuecomment-159670541
That seems fine, but this mechanism doesn't provide channels or any ability
for plugins to subscribe to messages. There's no reason to think that all
pl
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-159695223
Ok the next thing we need to do is remove the ```cert_files_vec``` from all
the functions in ```SSLUtils.cc```. The way we can do this is to add another
callback
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/351#issuecomment-159765613
I quickly scanned the diff. The API changes will still need to go through
API review on dev@. Documentation for the new APIs should be done as an API man
page in
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/356#issuecomment-160765501
Can you extract the guts of ```validate_hdr_host``` into a separate
function and use that to add a regression test host validation?
Are you sure that the
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/356#issuecomment-160855795
> so split out validate_hdr_field which takes a char const* and make a
regression test for that?
yup
---
If your project is set up for it, you can re
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/343#issuecomment-160858064
Looks nice. You don't need to hand-marshall the local manager message. Just
use ```mgmt_message_length()``` and ```mgmt_message_marshall()``` to marshall
i
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/343#issuecomment-161019315
Message marshaling works just fine, see #301 for an example of exactly what
you want to do here. If you can show me the problem then I can try to help.
---
If
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161024984
The whole ```ContFlags``` class seems unnecessary. If you really need
per-thread flags, then why not just keep a thread-specify ```uintptr_t``` in
the
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161030286
Yeh I mean the latter. All the code in ```ContFlags``` boils down to
getspecific + setspecific, the rest is really not necessary. A separate file
with a class
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/324#issuecomment-161183921
I filed this as [TS-4047](https://issues.apache.org/jira/browse/TS-4047).
---
If your project is set up for it, you can reply to this email and have your
reply
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/324#issuecomment-161188083
This is getting closer. Please rebase your changes onto latest master and
force push the branch to refresh.
There are a number of places that still need
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-161872963
This looks pretty good. in a previous comment I suggested sending a flags
field with the child rollback registration. Maybe that could be a way to
request that
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/362#issuecomment-161873548
FYI it is better to squash the format commits rather than have them appear
separately.
---
If your project is set up for it, you can reply to this email and have
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-162030676
It looks to me like the purpose of that assertion is to ensure that
```Diags::setup_diagslog``` is not called multiple times. If it was called
multiple times, the
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/359#issuecomment-162380212
Hi John. First, let's clean up the commit history on this branch.
- please squash the "clang format" and "updated comments" commi
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/359#issuecomment-163112743
We are almost there. To get the commits in the right order we can squash
commit 3 into commit 1 and remove the accidental changes to
```lib/tsconfig
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/363#issuecomment-163487199
AFAICT the assertion in ```setup_diagslog``` is correct. I would even go so
far as to make it an ```ink_release_assert``` because otherwise we would leak
the
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/359#issuecomment-163500832
Thanks @jrushf1239k. Now that we have a pretty clean commit on this branch,
I think that it is OK to make any additional changes in new commits.
I think
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/369#issuecomment-163998438
@jrushf1239k I think this will land before #359, so you will have to rebase.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-164817652
Thanks @zizhong
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/359#issuecomment-165243856
I haven't had time to review again,
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/359#issuecomment-165326498
> Note that FindParent() used to take a ParentConfigParams. Since the
lookup policy is now spread
> over the ParentRecord and the the config_params stru
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/379#issuecomment-165670171
Maybe ``AS_ESCAPE``?
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/397#issuecomment-166789440
What is ```ScopedContinuationLock``` for?
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/397#issuecomment-166795111
I just meant that I didn't see it used in this patch. Did I look in the
right place?
---
If your project is set up for it, you can reply to this email and
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/386#issuecomment-166942353
+1 after fixing review comments. We should aim to not have to use
```OPENSSL_IS_BORINGSSL```.
---
If your project is set up for it, you can reply to this email
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/397#issuecomment-166948715
Ah. Yep that seems fair enough.
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/386#issuecomment-167155989
Oh that's a bit nasty, we should add a comment to that effect. I think it
would be best to consolidate the openssl includes in one header so we can
capture
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-168268438
> Ideally I would refactor that method to separate context initialization
and configuration from
> inserting the context into the SSLCertLookup
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/403#issuecomment-168425132
@shukitchan why does the crash happen?
---
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
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/403#issuecomment-168721925
So it is a bug in ``TSDebug`` then? Can we fix that instead or as well?
---
If your project is set up for it, you can reply to this email and have your
reply
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/403#issuecomment-168776716
Why do you put the message in square brackets? That seems unusual. +1 at
any rate.
---
If your project is set up for it, you can reply to this email and have
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/394#issuecomment-168836153
This still returns time in seconds, since the arithmetic is the same. If
you change the time units should you add a new API to preserve compatibility?
---
If
1 - 100 of 680 matches
Mail list logo
