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.

Sign up for GitHub

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

Jump to bottom

REFACTOR: Change cachelist and alterList parsing logic. #600

Merged
merged 1 commit into from
Sep 26, 2024
Merged

REFACTOR: Change cachelist and alterList parsing logic. #600

merged 1 commit into from
Sep 26, 2024

Conversation

brido4125
Copy link
Collaborator

@brido4125 brido4125 commented Apr 21, 2023 •
edited
Loading

https://github.com/jam2in/arcus-works/issues/379

  • 위 이슈 기반의 PR입니다.

변경 사항

기존에는 String 타입으로 cacheListChange 및 alterListChange를 관리한다.
이러한 경우에는 ArcusClientPool 내에 있는 모든 ArcusClient들이
String -> List<InetSocketAddress>로의 변환을 반복한다.

이러한 반복을 제거하기 위해 znode로부터 List<String>을
받았을 때 List<InetSocketAddrress>로 바로 변환하도록 변경한다.

@brido4125 brido4125 self-assigned this Apr 21, 2023
jhpark816

This comment was marked as outdated.

Sign in to view

@jhpark816

This comment was marked as outdated.

Sign in to view
jhpark816

This comment was marked as outdated.

Sign in to view

@jhpark816

This comment was marked as outdated.

Sign in to view
@brido4125 brido4125 changed the base branch from master to develop April 25, 2023 06:25
@brido4125

This comment was marked as outdated.

Sign in to view
jhpark816

This comment was marked as outdated.

Sign in to view

@jhpark816
Copy link
Collaborator

@brido4125 develop 브랜치의 최신 코드 기준으로 rebase 바랍니다.

jhpark816
jhpark816 reviewed Apr 26, 2023
View reviewed changes
Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

일부 리뷰.

test 일부는 리뷰하지 않았는 데, 유사 형태는 함께 수정 바랍니다.

src/main/java/net/spy/memcached/AddrUtil.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/AddrUtil.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/CacheManager.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/CacheManager.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/CacheManager.java Outdated Show resolved Hide resolved
src/test/java/net/spy/memcached/ArcusKetamaNodeLocatorTest.java Outdated Show resolved Hide resolved
src/test/java/net/spy/memcached/BinaryIPV6ClientTest.java Outdated Show resolved Hide resolved
src/test/java/net/spy/memcached/BinaryIPV6ClientTest.java Outdated Show resolved Hide resolved
jhpark816
jhpark816 reviewed Apr 27, 2023
View reviewed changes
Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

일부 리뷰

src/test/java/net/spy/memcached/ArcusKetamaNodeLocatorTest.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
@brido4125 brido4125 force-pushed the refactor/commandCacheListChange branch 2 times, most recently from a72266a to 7d3eb4a Compare April 27, 2023 07:38
@brido4125

This comment was marked as outdated.

Sign in to view
@jhpark816
Copy link
Collaborator

merge 가능으로 보입니다.

jhpark816
jhpark816 reviewed Apr 27, 2023
View reviewed changes
Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
@brido4125
Copy link
Collaborator Author

  • finally문을 통해 일부 InetSocketAddress 만 들어가는 로직 수정했습니다.

jhpark816
jhpark816 reviewed Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusReplNodeAddress.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/CacheManager.java Outdated Show resolved Hide resolved
@brido4125 brido4125 force-pushed the refactor/commandCacheListChange branch from eec03df to 47e16bf Compare April 28, 2023 06:06
@jhpark816
Copy link
Collaborator

@uhm0311 @oliviarla 이제 리뷰 바랍니다.

uhm0311
uhm0311 previously approved these changes May 3, 2023
View reviewed changes
@jhpark816
Copy link
Collaborator

@brido4125
하나만 더 확인 바랍니다.
기존 메소드에서 아래 메소드 호출하도록 변경하면, 리턴 방식이 일부 변경(exception => emtpy list)될 것입니다.
따라서 아래 메소드를 호출하여 사용하는 코드에서는 이러한 변경이 있어도 문제가 없는 지를 검토 바랍니다.

List<InetSocketAddress> getAddresses(List<String> addressList)

@brido4125

This comment was marked as outdated.

Sign in to view
@brido4125 brido4125 force-pushed the refactor/commandCacheListChange branch 3 times, most recently from ecd8817 to 518724e Compare May 24, 2024 02:48
@brido4125 brido4125 marked this pull request as ready for review May 24, 2024 02:58
@brido4125 brido4125 force-pushed the refactor/commandCacheListChange branch from 518724e to 8eba399 Compare May 30, 2024 00:06
uhm0311
uhm0311 previously approved these changes May 30, 2024
View reviewed changes
@brido4125 brido4125 requested a review from jhpark816 June 5, 2024 07:22
@brido4125 brido4125 force-pushed the refactor/commandCacheListChange branch 2 times, most recently from 56c74df to ee4625d Compare September 25, 2024 07:36
@brido4125
Copy link
Collaborator Author

@uhm0311 @oliviarla

젤 위의 PR 설명 참고하셔서 리뷰 한번 부탁드립니다.

@brido4125 brido4125 changed the title REFACTOR: Change cacheListUpdate parsing logic. REFACTOR: Change cachelist and alterList parsing logic. Sep 25, 2024
uhm0311
uhm0311 reviewed Sep 25, 2024
View reviewed changes
src/main/java/net/spy/memcached/CacheManager.java Outdated
return getSocketAddressList(addrs.toString());
}

private List<InetSocketAddress> getSocketAddressList(String addrs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

getAddressListString() 메소드와 합치는 것은 추후에 하기로 했었나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

스트림 사용해서 getSocketAddressList 내부 로직 바꿀 때 같이 변경하려고 합니다.

Copy link
Collaborator

@oliviarla oliviarla Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jhpark816
이부분은 지금 같이 구현되면 좋을 것 같은데 어떠신가요?
getAddressListString 메서드의 반환 타입에 String을 찾아볼 수가 없는 구조라서 좀 이상합니다.

getSocketAddressList 내부에서 List -> String으로 변경하는 기존의 getAddressListString 메서드를 호출한 후 List를 반환하도록 하고, 외부에서는 getSocketAddressList 메서드만 호출하면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@oliviarla

함수 네이밍이랑 내부 동작이랑 일치 시켰습니다.
내부적인 리팩토링은 스트림 사용해서 진행할 건데,
앞선 코멘트에 의해
#600 (comment)
PR이 머지되는데 시간이 좀 걸릴 것 같아 반영하지 않았습니다.

왜냐하면 본 변경이 locator 작업의 첫 단계라 지연되지 않았으면 합니다
https://github.com/jam2in/arcus-works/issues/549#issuecomment-2373414131

uhm0311
uhm0311 previously approved these changes Sep 25, 2024
View reviewed changes
@jhpark816
Copy link
Collaborator

@oliviarla 리뷰 바랍니다.

@brido4125 brido4125 force-pushed the refactor/commandCacheListChange branch from ee4625d to f41c7c6 Compare September 26, 2024 00:30
oliviarla
oliviarla previously approved these changes Sep 26, 2024
View reviewed changes
uhm0311
uhm0311 previously approved these changes Sep 26, 2024
View reviewed changes
jhpark816
jhpark816 reviewed Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

src/main/java/net/spy/memcached/CacheManager.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/CacheManager.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/MemcachedConnection.java Outdated Show resolved Hide resolved
@brido4125 brido4125 dismissed stale reviews from uhm0311 and oliviarla via a4d19fa September 26, 2024 03:22
@brido4125 brido4125 force-pushed the refactor/commandCacheListChange branch from f41c7c6 to a4d19fa Compare September 26, 2024 03:22
@jhpark816 jhpark816 merged commit 3eef297 into naver:develop Sep 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@namsic namsic namsic left review comments

@jhpark816 jhpark816 jhpark816 approved these changes

@uhm0311 uhm0311 uhm0311 left review comments

@oliviarla oliviarla oliviarla left review comments

Assignees

@brido4125 brido4125

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brido4125 @jhpark816 @namsic @uhm0311 @oliviarla