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.

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

DRILL-5803: Show the hostname for each minor fragment in operator table #954

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@kkhatua
Copy link
Contributor

commented Sep 22, 2017

Based on DRILL-4909, list hostnames for each minor fragment within an operator table to identify any problematic host

@kkhatua

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

@paul-rogers Can you please review this minor UI update?

@paul-rogers
Copy link
Contributor

left a comment

Nice feature! Very handy.
Key comment is a suggestion to simplify the implementation over the increasingly-convoluted lists-of-pairs implementation we use now.

@@ -75,24 +75,31 @@ public ProfileWrapper(final QueryProfile profile) {
final List<OperatorWrapper> ows = new ArrayList<>();
// temporary map to store (major_id, operator_id) -> [(op_profile, minor_id)]
final Map<ImmutablePair<Integer, Integer>, List<ImmutablePair<OperatorProfile, Integer>>> opmap = new HashMap<>();
// temporary map to store (major_id, operator_id) -> [minorFragHostname]

This comment has been minimized.

Copy link
@paul-rogers

paul-rogers Sep 24, 2017

Contributor

Effectively, what this is doing is converting the opmap from a map of immutable pairs to a map of immutable triples. About here, we might want to restructure the map to point to a class which holds the two elements in the first pair, plus the host name.

Why? Simpler. Easier to keep class fields in sync than to keep two separate lists in sync.

This comment has been minimized.

Copy link
@kkhatua

kkhatua Sep 26, 2017

Author Contributor

+1 on this comment. However, I didn't what to make the PR look to radically alter what is already there. I'll incorporate the change. This should go nicely with a fix for DRILL-5802 and make life a bit easier

DRILL-5803: Show the hostname for each minor fragment in operator table
Based on DRILL-4909, list hostnames for each minor fragment within an operator table to identify any problematic host

@kkhatua kkhatua force-pushed the kkhatua:DRILL-5803 branch from 45b2271 to 8e89eb6 Sep 26, 2017

@kkhatua

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2017

@paul-rogers Made the changes

@paul-rogers

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2017

The fixes are an improvement. In some future commit, an actual class would be better than nested immutable pairs. At present, the code has become a bit cryptic and Python-like. But, we can do that improvement later.
+1

@asfgit asfgit closed this in dfd33e1 Oct 2, 2017

ilooner pushed a commit to ilooner/drill that referenced this pull request Oct 18, 2017

DRILL-5803: Show the hostname for each minor fragment in operator table
Based on DRILL-4909, list hostnames for each minor fragment within an operator table to identify any problematic host

closes apache#954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.