Skip to content

Completes code comment for redirect configs#4131

Merged
d2r merged 1 commit intoapache:masterfrom
d2r:records-config-redir-no-port-code-comment
Aug 21, 2018
Merged

Completes code comment for redirect configs#4131
d2r merged 1 commit intoapache:masterfrom
d2r:records-config-redir-no-port-code-comment

Conversation

@d2r
Copy link
Copy Markdown
Contributor

@d2r d2r commented Aug 18, 2018

While looking into other changes planned for the 8.x release, I found a simple inconsistency in a code comment.

I wanted to fix this in a separate PR first, so that if something happens to other unrelated changes, this simple correction can remain separate. That is why it is requested to be back-ported to 8.x.

@d2r d2r added the Cleanup label Aug 18, 2018
@d2r d2r added this to the 9.0.0 milestone Aug 18, 2018
@d2r d2r self-assigned this Aug 18, 2018
Comment thread mgmt/RecordsConfig.cc
//# 1. number_of_redirections: The maximum number of redirections TS permits. Disabled if set to 0 (default)
//# 2. proxy.config.http.redirect_use_orig_cache_key: Location Header if set to 0 (default), else use original request cache key
//# 3. post_copy_size: The maximum POST data size TS permits to copy
//# 3. redirection_host_no_port: do not include default port in host header during redirection
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: This text was taken from HttpConfig.cc.

@maskit
Copy link
Copy Markdown
Member

maskit commented Aug 20, 2018

Shouldn't we remove either of them? Keeping two leaves a risk of inconsistency. I'd rather refer one from the other.

@d2r
Copy link
Copy Markdown
Contributor Author

d2r commented Aug 20, 2018

Shouldn't we remove either of them? Keeping two leaves a risk of inconsistency. I'd rather refer one from the other.

@maskit I want to be sure I understand. I am adding what looks like a missing code comment and renumbering the sequence.

It seemed to me that a new config variable was added at some point in the past, but the code comment for this config was not added above the group of definitions. This is adding the missing code comment and renumbering the sequence so it corresponds to the definitions, in the order that their declarations appear.

Here is the complete section after the proposed change:

  //#
  //# Redirection
  //#
  //# 1. number_of_redirections: The maximum number of redirections TS permits. Disabled if set to 0 (default)
  //# 2. proxy.config.http.redirect_use_orig_cache_key: Location Header if set to 0 (default), else use original request cache key
  //# 3. redirection_host_no_port: do not include default port in host header during redirection
  //# 4. post_copy_size: The maximum POST data size TS permits to copy
  //#
  //##############################################################################
  {RECT_CONFIG, "proxy.config.http.number_of_redirections", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
  ,
  {RECT_CONFIG, "proxy.config.http.redirect_use_orig_cache_key", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
  ,
  {RECT_CONFIG, "proxy.config.http.redirect_host_no_port", RECD_INT, "1", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
  ,
  {RECT_CONFIG, "proxy.config.http.post_copy_size", RECD_INT, "2048", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,

Are you saying we should remove some of the code comments here?

@maskit
Copy link
Copy Markdown
Member

maskit commented Aug 20, 2018

Are you saying we should remove some of the code comments here?

Yes, if the two comment blocks are exactly the same.

@d2r
Copy link
Copy Markdown
Contributor Author

d2r commented Aug 20, 2018

Yes, if the two comment blocks are exactly the same.

Ah I see! From what I remember, they are not exactly the same, but I agree we should not duplicate comments unnecessarily. Let me check on it.

@d2r d2r added the WIP label Aug 20, 2018
@d2r d2r force-pushed the records-config-redir-no-port-code-comment branch from 34ccd82 to 7e0bece Compare August 20, 2018 16:43
@d2r
Copy link
Copy Markdown
Contributor Author

d2r commented Aug 20, 2018

@maskit @SolidWallOfCode
Updated this PR to de-duplicate the code comments as well.

  1. I chose to keep the comments in RecordsConfig rather than HttpConfig, since the defaults and other attributes are defined in the former. I did not think it was better to refer to default settings that were defined in another file. So I pointed HttpConfig at RecordsConfig.
  2. Where there were differences in comments, I prefered the ones in RecordsConfig. I did not see much of a substantial difference in meaning, so I opted to keep the existing comments the same where I could.

@d2r d2r removed the WIP label Aug 20, 2018
Copy link
Copy Markdown
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update.

@d2r d2r merged commit 48e0565 into apache:master Aug 21, 2018
@bryancall bryancall modified the milestones: 9.0.0, 8.0.0 Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants