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

Provide a way to configure a CacheSpec for HttpFileService #2142

Merged
merged 20 commits into from Oct 10, 2019

Conversation

@heowc
Copy link
Contributor

heowc commented Oct 2, 2019

Motivation:

A user cannot set a way to configure a CacheSpec for HttpFileService.

Modifications:

  • Add a new flag com.linecorp.armeria.fileServiceCache=<spec>
  • Add entryCacheSpec for HttpFileServiceBuilder
  • Delegate maxCacheEntries to entryCacheSpec

Result:

@heowc heowc marked this pull request as ready for review Oct 3, 2019
@heowc heowc requested review from ikhoon, minwoox and trustin as code owners Oct 3, 2019
@heowc heowc changed the title [WIP] Provide a way to configure a CacheSpec for HttpFileService Provide a way to configure a CacheSpec for HttpFileService Oct 3, 2019
@trustin trustin added the new-feature label Oct 4, 2019
@trustin trustin added this to the 0.94.1 milestone Oct 4, 2019
Fix code
- Rename `httpFileServiceCache` to `fileServiceCache`
- Fix javadoc
- Fix `checkState`
- Rename fields
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #2142 into master will decrease coverage by 0.01%.
The diff coverage is 61.76%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2142      +/-   ##
============================================
- Coverage     73.59%   73.58%   -0.02%     
- Complexity     9566     9574       +8     
============================================
  Files           837      837              
  Lines         36830    36883      +53     
  Branches       4542     4548       +6     
============================================
+ Hits          27104    27139      +35     
- Misses         7396     7417      +21     
+ Partials       2330     2327       -3
Impacted Files Coverage Δ Complexity Δ
.../linecorp/armeria/server/file/HttpFileService.java 83.08% <100%> (-0.13%) 39 <1> (ø)
...c/main/java/com/linecorp/armeria/common/Flags.java 61.67% <100%> (+0.51%) 53 <1> (+1) ⬆️
...rp/armeria/server/file/HttpFileServiceBuilder.java 64.4% <52.94%> (-3.68%) 17 <2> (+1)
...orp/armeria/server/file/HttpFileServiceConfig.java 57.89% <54.54%> (-1.49%) 12 <3> (+1)
...rp/armeria/server/tomcat/ManagedTomcatService.java 50.84% <0%> (-22.04%) 6% <0%> (-1%)
...armeria/server/tomcat/Tomcat90ProtocolHandler.java 48.14% <0%> (-14.82%) 11% <0%> (-4%)
...corp/armeria/client/DefaultEventLoopScheduler.java 79.27% <0%> (-9.91%) 30% <0%> (-5%)
...inecorp/armeria/client/AbstractEventLoopState.java 92.3% <0%> (-7.7%) 6% <0%> (-1%)
...com/linecorp/armeria/client/OneEventLoopState.java 66.66% <0%> (-4.77%) 6% <0%> (-1%)
.../armeria/client/endpoint/dns/DnsEndpointGroup.java 83.15% <0%> (-3.16%) 13% <0%> (-2%)
... and 26 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 d4e8f82...58c315d. Read the comment docs.

@heowc heowc requested a review from trustin Oct 7, 2019
heowc added 2 commits Oct 7, 2019
Fix code
- Fix javadoc
- Fix doc
- Revert test
- Remove `@deprecated`
@heowc heowc requested review from ikhoon and trustin Oct 8, 2019
@trustin
trustin approved these changes Oct 8, 2019
Copy link
Member

trustin left a comment

LGTM once documentation nits and @minwoox's comments are addressed. 👍

@ikhoon
ikhoon approved these changes Oct 8, 2019
Copy link
Contributor

ikhoon left a comment

Nice work, I look forward to your next contribution 😊

/**
* Sets the cache spec for caching file entries. If not set, {@code "maximumSize=1024"} is used by default.
*/
public HttpFileServiceBuilder entryCacheSpec(String entryCacheSpec) {

This comment has been minimized.

Copy link
@ikhoon

ikhoon Oct 8, 2019

Contributor

I just saw that code patch coverage is lower than the baseline. Could you add a test case for here :-)

This comment has been minimized.

Copy link
@heowc

heowc Oct 9, 2019

Author Contributor

I added test case. Please tell me if it is not enough.

Fix code
- Fix javadoc
- Move `validateMaxCacheEntries(...)`
@minwoox
minwoox approved these changes Oct 8, 2019
Copy link
Member

minwoox left a comment

Great job, @heowc!

heowc added 2 commits Oct 9, 2019
@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Oct 9, 2019

I found a case where it wasn't checked normally during the existing test. baseUri contains /cached rather than/cached/. 58808d4

@minwoox

This comment has been minimized.

Copy link
Member

minwoox commented Oct 10, 2019

Found build failure:

com.linecorp.armeria.server.file.HttpFileServiceTest > testClassPathGetFromModule(String)[1] FAILED
    java.lang.AssertionError: 
    Expecting:
     <["/cached/by-entry/classes/java/lang/Object.class",
        "/cached/classes/java/lang/Object.class"]>
    to contain:
     <["/cached/by-entry/classes/java/lang/Object.class"]>
    but could not find:
     <["/cached/by-entry/classes/java/lang/Object.class"]>
        at 
@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Oct 10, 2019

Let's check a little bit more. 😓

heowc added 2 commits Oct 10, 2019
@trustin trustin merged commit 84d3b36 into line:master Oct 10, 2019
1 check failed
1 check failed
continuous-integration/appveyor/pr AppVeyor build failed
Details
@trustin

This comment has been minimized.

Copy link
Member

trustin commented Oct 10, 2019

Thank you, @heowc!

@heowc heowc deleted the heowc:provide-cache-spec-for-httpfileservice branch Oct 11, 2019
eugene70 pushed a commit to eugene70/armeria that referenced this pull request Oct 16, 2019
Motivation:

A user cannot set a way to configure a CacheSpec for HttpFileService.

Modifications:

- Add a new flag `com.linecorp.armeria.fileServiceCache=<spec>`
- Add `entryCacheSpec` for `HttpFileServiceBuilder`
- Delegate `maxCacheEntries` to `entryCacheSpec`

Result:

- Fixes line#916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.