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

Upd/Generics have landed #2340

Merged
merged 8 commits into from
May 19, 2023

Conversation

notimetoname
Copy link
Contributor

@notimetoname notimetoname commented May 12, 2023

Closes #2181. Blocked by #2342.

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #2340 (a288668) into master (b282a8f) will increase coverage by 0.01%.
The diff coverage is 7.69%.

❗ Current head a288668 differs from pull request most recent head 3cdec55. Consider uploading reports for the commit 3cdec55 to get more accurate results

@@            Coverage Diff             @@
##           master    #2340      +/-   ##
==========================================
+ Coverage   29.87%   29.88%   +0.01%     
==========================================
  Files         398      398              
  Lines       29825    29806      -19     
==========================================
- Hits         8909     8907       -2     
+ Misses      20199    20182      -17     
  Partials      717      717              
Impacted Files Coverage Δ
cmd/neofs-node/cache.go 0.00% <0.00%> (ø)
..._object_storage/blobstor/blobovniczatree/delete.go 65.07% <0.00%> (ø)
...cal_object_storage/blobstor/blobovniczatree/get.go 62.12% <0.00%> (ø)
pkg/morph/client/client.go 0.00% <ø> (ø)
pkg/morph/client/constructor.go 0.00% <0.00%> (ø)
pkg/morph/client/notary.go 0.00% <0.00%> (ø)
pkg/services/object_manager/placement/netmap.go 0.00% <0.00%> (ø)
pkg/services/policer/policer.go 0.00% <0.00%> (ø)
pkg/services/policer/process.go 0.00% <0.00%> (ø)
pkg/services/tree/cache.go 0.00% <0.00%> (ø)
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@roman-khimov
Copy link
Member

See #2319 for things to fix when dropping 1.17 support. There is #2181, but I'm not so immediately sure about that.

@roman-khimov
Copy link
Member

Can we split it to #2319 and #2181 parts?

@notimetoname notimetoname marked this pull request as draft May 15, 2023 08:49
@notimetoname
Copy link
Contributor Author

#2342 is done. Lets have generic fun.

@notimetoname notimetoname marked this pull request as ready for review May 15, 2023 18:42
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Can't say I'm impressed with the diffstat though (+137 −159), my expectations were somewhat higher. Something tells me we'll need to revise these caches anyway.

Please add #2181 fix/close somewhere.

cmd/neofs-node/cache.go Show resolved Hide resolved
cmd/neofs-node/cache.go Show resolved Hide resolved
cmd/neofs-node/cache.go Show resolved Hide resolved
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
@notimetoname
Copy link
Contributor Author

notimetoname commented May 18, 2023

Please add #2181 fix/close somewhere.

@roman-khimov, added to PR's description. It is 8 commits now, not sure which of them should have that link (or all?).

Can't say I'm impressed with the diffstat though (+137 −159), my expectations were somewhat higher.

In fact, mine was too. But i like that we do not have casts (ok, "assertions") in that code anymore, i find that parentheses dance (4+ in one expression) hard to read. Also, i would say that generics just do their work, and if it can be done with a small amount of code it should be done. In other words, i would say that our caches are too complex, so generics do not make them shorter where nothing relates to "generic code".

Anyway, i think, LRU caches will be updated to v2 somehow, so the issue says to update it, that PR updates it. Any other refactor should be considered in a separate way (can create an issue about that?).

@cthulhu-rider cthulhu-rider merged commit 86d3b57 into nspcc-dev:master May 19, 2023
@notimetoname notimetoname deleted the upd/generics-have-landed branch May 19, 2023 10:42
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.

Use generics for morph caches
3 participants