Skip to content

Web console: replace (and remove) old consoles#8838

Merged
fjy merged 20 commits intoapache:masterfrom
implydata:rm-old-console
Nov 16, 2019
Merged

Web console: replace (and remove) old consoles#8838
fjy merged 20 commits intoapache:masterfrom
implydata:rm-old-console

Conversation

@vogievetsky
Copy link
Copy Markdown
Contributor

@vogievetsky vogievetsky commented Nov 7, 2019

Fixes #8150

This PR replaces and removes the legacy coordinator and overlord consoles and replaces them with the new console. The new console has been adapted to have smart capability detection with graceful fallback mode when running on services where not all the APIs are available.

image

Reasons for doing this:

  • To provide a much better experience to new users with fewer components to understand. This is especially true with coordiantor.asOverlord mode being the new config default.
  • The old consoles are not maintained and have a lot of "code rot" (the Coordiantor console has 10s of errors thrown in the JS console and the Overlord console uses a five year old version of jQuery with 23 LGTM alerts)
  • The old consoles do not look good and are not visually consistent with each other.
  • Fewer dependancies. The old consoles depend on some really old libraries.

I believe that the new console has full functional parity with what it is replacing but if I missed anything speak up and it will be addressed ASAP.

@vogievetsky vogievetsky changed the title Web console: replace (and remove) old consoles with new console Web console: replace (and remove) old consoles Nov 7, 2019
@clintropolis
Copy link
Copy Markdown
Member

+1000

(not sure if my review counts since i made one of the commits 😜)

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 7, 2019

This pull request fixes 28 alerts when merging 1f6a9e2 into 517c146 - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Nov 7, 2019

@vogievetsky I think this is failing web console UT

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 7, 2019

This pull request fixes 28 alerts when merging 7eb8bb1 into 517c146 - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

@clintropolis
Copy link
Copy Markdown
Member

On the java side of things, I added a new WebConsoleJettyServerInitializer utility method that takes a ServletContextHandler and sets it up for serving the new web console, so that the same initialization code is now shared between router, coordinator, and overlord.

Additionally, there was a coordinator config that looked like it allowed specifying a custom location for the newer old coordinator console for development purposes, that has been removed.

@vogievetsky
Copy link
Copy Markdown
Contributor Author

@clintropolis could you please add these redirects to Jetty:

/index.html -> /unified-console.html
/old-console/index.html -> /unified-console.html
/console.html -> /unified-console.html

@clintropolis
Copy link
Copy Markdown
Member

clintropolis commented Nov 8, 2019

@vogievetsky I have added the redirects from the old pages. While doing this I noticed that the 'coordinator mode' actually is not working if the overlord is down, presumably something related to the secret overlord proxy that it has if it is not running in the combined coordinator+overlord mode. Anyway, it makes the 'coordinator mode' totally non-functional, so I think we need to fix that.

Screen Shot 2019-11-08 at 12 52 31 AM

Screen Shot 2019-11-08 at 12 52 53 AM

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 8, 2019

This pull request fixes 28 alerts when merging 21ff6e0 into c204d68 - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

@vogievetsky
Copy link
Copy Markdown
Contributor Author

Thanks @clintropolis fixed and pushed.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 8, 2019

This pull request fixes 28 alerts when merging 8ff3c9c into 6eacaf4 - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

Comment thread licenses.yaml Outdated
See the Apache Version 2.0 License for specific language governing permissions
and limitations under the License.
***************************************************************************** */
name: "tslib"
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.

are there 2 versions of tslib? And does this version not have the NOTICE file?

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.

So that removed notice was removed because this is an auto generated block. It looks like you added the notice: key in https://github.com/apache/incubator-druid/pull/8306/files#diff-66ffa06e81466a207a1dad823ec2fe90R5049 but script/licenses which is what makes this block is not aware of it. What is the deal here? What does the notice: key represent and should script/licenses be updated to generate it?

@clintropolis
Copy link
Copy Markdown
Member

What is the deal here? What does the notice: key represent and should script/licenses be updated to generate it?

The notice sections are manually maintained, and in fact must be, because we don't automatically include the entire notice, rather the part that is relevant to the code being used by Druid. These parts generate Druids own source and binary NOTICE files at distribution build time, so are really important to be correct and maintained.

I think we should probably modify the script to indicate to whoever runs it whenever a version of a dependency is updated that had a notice entry so that it can be manually updated. We went through all of the web dependencies as part of previous releases, but whenever we add new dependencies we should ensure that if they have a NOTICE file that the relevant contents make it into licenses.yaml. I don't think this script modification needs to be done as part of this PR, but we should at least preserve (and probably update) the notice entry for tslib.

@vogievetsky
Copy link
Copy Markdown
Contributor Author

I edited script/licenses to make sure it preserves the notice and dedupes the libraries

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 10, 2019

This pull request fixes 28 alerts when merging f98b954 into 75ea0d5 - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 12, 2019

This pull request fixes 28 alerts when merging 14f5e93 into df2f77c - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

1 similar comment
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 12, 2019

This pull request fixes 28 alerts when merging 14f5e93 into df2f77c - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 12, 2019

This pull request fixes 28 alerts when merging e026739 into df2f77c - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 13, 2019

This pull request fixes 28 alerts when merging d22c178 into a066cc5 - view on LGTM.com

fixed alerts:

  • 25 for Unused variable, import, function or class
  • 1 for Useless assignment to local variable
  • 1 for Comparison between inconvertible types
  • 1 for Incomplete string escaping or encoding

Copy link
Copy Markdown
Contributor

@ccaominh ccaominh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vogievetsky
Copy link
Copy Markdown
Contributor Author

@fjy fjy merged commit 17d773d into apache:master Nov 16, 2019
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
@jon-wei jon-wei mentioned this pull request Dec 28, 2019
@vogievetsky vogievetsky deleted the rm-old-console branch October 16, 2020 05:35
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.

Make the Unified web console available on Coordinator

6 participants