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

Removing all restlet stuff from code #1833

Merged
merged 11 commits into from Aug 29, 2017
Merged

Conversation

mcvsubbu
Copy link
Contributor

@mcvsubbu mcvsubbu commented Aug 23, 2017

Removed all references to restlet, including pom files.
Had to remove the LLCSegmentCommitTest since that was using the internals of restlet mechanism to verify the code.

Copy link
Member

@jenniferdai jenniferdai left a comment

Choose a reason for hiding this comment

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

Congratulations!!!

@codecov-io
Copy link

codecov-io commented Aug 23, 2017

Codecov Report

Merging #1833 into master will increase coverage by 2.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1833      +/-   ##
==========================================
+ Coverage   66.18%   68.78%   +2.59%     
==========================================
  Files         885      853      -32     
  Lines       42716    40673    -2043     
  Branches     5615     5318     -297     
==========================================
- Hits        28273    27977     -296     
+ Misses      12565    10839    -1726     
+ Partials     1878     1857      -21
Impacted Files Coverage Δ
...inkedin/pinot/controller/util/TableSizeReader.java 91.66% <ø> (ø) ⬆️
.../linkedin/pinot/controller/api/pojos/Instance.java 38.18% <ø> (ø) ⬆️
...ler/api/resources/PinotSegmentRestletResource.java 27.48% <ø> (ø) ⬆️
.../pinot/server/api/resources/MmapDebugResource.java 0% <ø> (ø) ⬆️
.../pinot/controller/util/SegmentCompletionUtils.java 75% <ø> (+2.77%) ⬆️
...er/api/resources/LLCSegmentCompletionHandlers.java 57.37% <100%> (+0.35%) ⬆️
...m/linkedin/pinot/controller/ControllerStarter.java 58.16% <100%> (-5.52%) ⬇️
...nkedin/pinot/common/metrics/ValidationMetrics.java 15.94% <0%> (-59.43%) ⬇️
...java/com/linkedin/pinot/core/query/utils/Pair.java 63.63% <0%> (-36.37%) ⬇️
...pinot/controller/validation/ValidationManager.java 52.05% <0%> (-29.23%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 307b669...7c423cc. Read the comment docs.

@Jackie-Jiang
Copy link
Contributor

Love the code coverage improvement 👍

Copy link
Contributor

@antumbde antumbde left a comment

Choose a reason for hiding this comment

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

One comment around removing todo. All other comments here and on review are optional

pinot-common/ restlet folder, package com.linkedin.pinot.common.restlet.swagger, has multiple annotations defined that are not used anymore. We can drop those as well.

Our top level NOTICE file has notice about restlet. We can remove that.

(optional) We should also drop the word restlet from jersey class names now

@@ -248,26 +248,6 @@
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

this pom has a restlet repository. We can remove that now

component.getServers().add(Protocol.HTTP, restletPort);
component.getClients().add(Protocol.FILE);
component.getClients().add(Protocol.JAR);
Context applicationContext = component.getContext().createChildContext();
LOGGER.info("Controller download url base: {}", config.generateVipUrl());
LOGGER.info("Injecting configuration and resource managers to the API context");
// TODO: Remove attributes map when removing restlet
Copy link
Contributor

Choose a reason for hiding this comment

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

we can drop the todo now that you have addressed it

@@ -81,8 +72,6 @@

public ControllerStarter(ControllerConf conf) {
config = conf;
component = new Component();
controllerRestApp = new ControllerRestApplication(config.getQueryConsole());
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) The executorservice threads are named as 'restlet....'. We can change that to avoid possible confusion

Had to remove one annotation in pojos/Instance.java to get things to compile.
@mcvsubbu mcvsubbu merged commit d096d7f into master Aug 29, 2017
@mcvsubbu mcvsubbu deleted the remove-old-controller-api branch August 29, 2017 01:32
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.

None yet

5 participants