Skip to content

nsqadmin: JS error with no clients - #642

Merged
jehiah merged 4 commits into
nsqio:masterfrom
mreiferson:nsqadmin_js_error_642
Aug 27, 2015
Merged

nsqadmin: JS error with no clients#642
jehiah merged 4 commits into
nsqio:masterfrom
mreiferson:nsqadmin_js_error_642

Conversation

@mreiferson

Copy link
Copy Markdown
Member

This started off as a fix to a bug we introduced in our recent nsqadmin
refactoring (#323)... and then I got carried away and fixed #627 and #341,
which required propagating certain metadata differently in the internal/clusterinfo
package as well as adding more basic metadata to nsqd's /info endpoint.

RFR @jehiah

@mreiferson

Copy link
Copy Markdown
Member Author

Here's what the hostname stuff (#341) looks like:

screen shot 2015-08-26 at 4 28 44 pm

Comment thread nsqd/http.go

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at what else is included in /stats should start_time or health be included here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmmm, one way of thinking about the difference between the endpoints is that /info returns "static" while /stats returns dynamic metadata.

If that's true, then I don't think we would add health but perhaps start_time though?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds reasonable. Since realizing that /stats can be very heavy weight i keep thinking of lighter ways to get some of that data. We do have topic & channel filtering there so that certainly helps.

@mreiferson

Copy link
Copy Markdown
Member Author

updated @jehiah

Comment thread nsqadmin/static/js/models/topic.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noticing from your screenshot, can you make the comparison of hostname case insensitive here and the other function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

updated

@jehiah

jehiah commented Aug 27, 2015

Copy link
Copy Markdown
Member

👍 🔨

@mreiferson
mreiferson force-pushed the nsqadmin_js_error_642 branch from ee17ac8 to 5bd6fa4 Compare August 27, 2015 18:40
@mreiferson

Copy link
Copy Markdown
Member Author

squashed

jehiah added a commit that referenced this pull request Aug 27, 2015
@jehiah
jehiah merged commit df1678d into nsqio:master Aug 27, 2015
@mreiferson
mreiferson deleted the nsqadmin_js_error_642 branch August 27, 2015 18:42
@mreiferson mreiferson added this to the nsqadmin refactoring milestone Aug 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It didn't occur to me at the time, but there is a compatibility issue with expecting this information back from the new /info endpoint. Namely w/o these fields nsqadmin is unable to display the topic or channel page for older hosts because it will attempt to query /info and use this empty field to query /stats.

[nsqadmin] 2015/09/21 13:37:11.922482 CI: querying nsqd http://127.0.0.1:4151/info
[nsqadmin] 2015/09/21 13:37:11.923098 CI: querying nsqd http://:0/stats?format=json
[nsqadmin] 2015/09/21 13:37:11.923515 ERROR: failed to get channel metadata - failed to query any nsqd

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was aware of this, however the /nodes page was already completely broken when configuring nsqd directly, so I didn't think it made any sense to try to be backwards compatible.

I suppose if you went directly to the page for a specific node you'd still encounter this issue?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For context, this change unexpectedly broke /topics/... and /topics/.../channel which previously worked.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nsqadmin: nodes says zero without lookupd configured

2 participants