Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TS-4868: Add configs back and cleanup other cluster init #1022

Merged
merged 1 commit into from
Sep 15, 2016
Merged

TS-4868: Add configs back and cleanup other cluster init #1022

merged 1 commit into from
Sep 15, 2016

Conversation

PSUdaemon
Copy link
Contributor

No description provided.

@PSUdaemon PSUdaemon added this to the 7.0.0 milestone Sep 15, 2016
@atsci
Copy link

atsci commented Sep 15, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/811/ for details.

@atsci
Copy link

atsci commented Sep 15, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/707/ for details.

jpeach
jpeach reviewed Sep 15, 2016
View reviewed changes
cmd/traffic_cop/traffic_cop.cc
@@ -657,9 +656,6 @@ config_reload_records()
config_read_int("proxy.config.cop.source_port", &source_port, true);
#endif

config_read_int("proxy.local.cluster.type", &tmp_int);
cluster_type = static_cast<MgmtClusterType>(tmp_int);

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove cluster_type altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another patch that I have as a WIP that is for totally removing it. It touches more things than it first appears.

mgmt/RecordsConfig.cc
{RECT_CONFIG, "proxy.config.cluster.cluster_load_clear_duration", RECD_INT, "24", RECU_NULL, RR_NULL, RECC_NULL, NULL, RECA_NULL}
,
{RECT_CONFIG, "proxy.config.cluster.cluster_load_exceed_duration", RECD_INT, "4", RECU_NULL, RR_NULL, RECC_NULL, NULL, RECA_NULL}
,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to add these back if there's no way to enable clustering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the problem that we ran into in the first place. We'd have to go through the code and match the defaults to the case if the config doesn't exist, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do the configs need to be added back in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a bunch of code that asserts on the existence. And the more we dig into it the more you have to remove, ala #966. This broke traffic_cop and traffic_manager without these configs.

@PSUdaemon
Copy link
Contributor Author

@jpeach see #966.

@gtenev
Copy link
Contributor

gtenev commented Sep 15, 2016

I have not looked enough to say if this change is necessary or not but just wanted to mention that it helped me with a crash I got after syncing my fork.

Starting program: /opt/apache/trafficserver/bin/traffic_manager
[Thread debugging using libthread_db enabled]
[E. Mgmt] log ==> [TrafficManager] using root directory '/opt/apache/trafficserver'
[New Thread 0x7ffff709b700 (LWP 24130)]
[New Thread 0x7fffee69a700 (LWP 24131)]

Program received signal SIGSEGV, Segmentation fault.
inet_network (cp=0x0) at inet_net.c:55
55          if (*cp == '0')
Missing separate debuginfos, use: debuginfo-install libunwind-1.1-3.el6.x86_64 tcl-8.5.7-6.el6.x86_64
(gdb) where
#0  inet_network (cp=0x0) at inet_net.c:55
#1  0x0000000000430273 in main (argc=<value optimized out>, argv=<value optimized out>) at traffic_manager.cc:668

Applying the patch seemed to fix the problem for me.

@dragon512
Copy link
Contributor

Fixes the issue I reported on

@dragon512
Copy link
Contributor

+1

@PSUdaemon PSUdaemon merged commit fe4a85d into apache:master Sep 15, 2016
gtenev
gtenev reviewed Sep 15, 2016
View reviewed changes
Copy link
Contributor

@gtenev gtenev left a comment

Choose a reason for hiding this comment

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

review test (by jpeach's request), please ignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryancall bryancall bryancall left review comments

@gtenev gtenev gtenev left review comments

@jpeach jpeach jpeach approved these changes

Assignees

@jpeach jpeach

@zwoop zwoop

Labels
Cleanup
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@PSUdaemon @atsci @gtenev @dragon512 @jpeach @bryancall @zwoop