Skip to content

STORM-3442 Add owner to supervisor summary#3067

Merged
Ethanlm merged 1 commit intoapache:masterfrom
dandsager1:STORM-3442
Jul 5, 2019
Merged

STORM-3442 Add owner to supervisor summary#3067
Ethanlm merged 1 commit intoapache:masterfrom
dandsager1:STORM-3442

Conversation

@dandsager1
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@Ethanlm Ethanlm left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Left some comments.

/**
* Get the integer value of this enum value, as defined in the Thrift IDL.
*/
@Override
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In case someone is curious (like me), #3020 modified these generated file (NumErrorsChoice.java and TopologyInitialStatus.java) directly. So the rerun of genthrift.sh in this PR overrides the change. And this is good.

}
},
{
data: 'owner',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this also add "owner" column to the topology summary page? If so, it will be redundant since the topology summary page already has "owner" information

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.

Correct. This also adds owner to the topology summary page. The duplicate owner can be removed at the cost of adding logic to script.js. It seems a wash to me.

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.

Removing duplicate owner is simple. Making this change and also removing "owner" tooltip from topology-page-template

true); //this is the topology page, so we know the user is authorized
true, //this is the topology page, so we know the user is authorized
null,
owner);
Copy link
Copy Markdown
Contributor

@Ethanlm Ethanlm Jul 3, 2019

Choose a reason for hiding this comment

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

With this change, the other aggWorkerStats function is no longer used by anyone and can be removed, or change it to

aggWorkerStats(String stormId, String stormName,
                                                     Map<Integer, String> task2Component,
                                                     Map<List<Integer>, Map<String, Object>> beats,
                                                     Map<List<Long>, List<Object>> exec2NodePort,
                                                     Map<String, String> nodeHost,
                                                     Map<WorkerSlot, WorkerResources> worker2Resources,
                                                     boolean includeSys, boolean userAuthorized, String owner) 

so by default filterSupervisor is null and then it can be used here.

But this is not very important.

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.

removing unused overload

Copy link
Copy Markdown
Contributor

@Ethanlm Ethanlm 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 fix. Travis JDK: openjdk11 Java MODULES=Server failed on GPG error. Doesn't seem related. Could you please squash the commits and I can then merge it

@Ethanlm Ethanlm merged commit 8d4f0c6 into apache:master Jul 5, 2019
@dandsager1 dandsager1 deleted the STORM-3442 branch July 5, 2019 19:48
@dandsager1 dandsager1 restored the STORM-3442 branch July 8, 2019 13:26
@dandsager1 dandsager1 deleted the STORM-3442 branch July 23, 2019 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants