Skip to content

CLEANUP: Refactor methods of setting ritems#747

Open
ing-eoking wants to merge 1 commit intonaver:developfrom
ing-eoking:ritem
Open

CLEANUP: Refactor methods of setting ritems#747
ing-eoking wants to merge 1 commit intonaver:developfrom
ing-eoking:ritem

Conversation

@ing-eoking
Copy link
Copy Markdown
Collaborator

@ing-eoking ing-eoking commented Mar 26, 2024

🔗 Related Issue

  • jam2in/arcus-works#493

⌨️ What I did

  • 추가 데이터를 담는 공간인 rlist를 추가했습니다.
  • ritem_set_first / ritem_set_next, RTYPE 를 제거했습니다.
  • 관련 함수 add_ritem / add_ritem_mblck / add_ritem_einfo / add_ritem_hinfo를 추가했습니다.
  • conn_nread를 변경된 방식에 맞게 수정했습니다.
  • rlist의 메모리 부족 처리를 추가하면서 일부 에러 핸들링을 아래의 PR처럼 통일시켰습니다.

@ing-eoking ing-eoking marked this pull request as draft March 26, 2024 02:17
@ing-eoking ing-eoking force-pushed the ritem branch 2 times, most recently from 26fb268 to 2470626 Compare March 26, 2024 08:11
@ing-eoking ing-eoking marked this pull request as ready for review March 26, 2024 08:15
@ing-eoking ing-eoking requested review from jhpark816 and namsic March 26, 2024 08:15
@jhpark816

This comment was marked as resolved.

Comment thread memcached.c Outdated
Comment thread memcached.c Outdated
@ing-eoking ing-eoking force-pushed the ritem branch 2 times, most recently from ef3e0ff to f1ab61b Compare March 27, 2024 00:11
@ing-eoking ing-eoking requested a review from namsic March 27, 2024 00:13
jhpark816

This comment was marked as resolved.

@ing-eoking ing-eoking marked this pull request as draft March 28, 2024 07:39
@ing-eoking ing-eoking self-assigned this Mar 29, 2024
@ing-eoking ing-eoking force-pushed the ritem branch 8 times, most recently from eed606d to 414953c Compare March 29, 2024 02:42
@ing-eoking ing-eoking marked this pull request as ready for review March 29, 2024 02:55
@ing-eoking

This comment was marked as resolved.

@ing-eoking

This comment was marked as resolved.

@namsic
Copy link
Copy Markdown
Collaborator

namsic commented Mar 29, 2024

add_ritem_e/hinfo 또한 로직은 add_iov_e/hinfo_ascii/bin와 유사하다고 생각됩니다.
add_ritem_e/hinfo_ascii/bin으로 분리할까요?
add_ritem_mblck는 둘다(ASCII, Binary) 같은 방식을 사용하므로 분리 X

지금 구현 기준으로는 protocol 간 코드 차이가 거의 없어서 분리하지 않아도 될 것 같습니다.

@ing-eoking ing-eoking marked this pull request as ready for review April 3, 2024 03:12
@jhpark816
Copy link
Copy Markdown
Collaborator

@namsic 리뷰 바랍니다.

Comment thread memcached.c Outdated
Comment thread memcached.c Outdated
@ing-eoking ing-eoking marked this pull request as draft April 3, 2024 08:18
@jhpark816
Copy link
Copy Markdown
Collaborator

@namsic
리뷰를 마쳤나요? approved 상태여서 물어봅니다.

@ing-eoking ing-eoking requested review from jhpark816 and namsic April 3, 2024 08:27
@namsic
Copy link
Copy Markdown
Collaborator

namsic commented Apr 3, 2024

@jhpark816 리뷰를 마쳤나요? approved 상태여서 물어봅니다.

예. 리뷰 마쳤습니다.
개인적으로 아래 코드를 더 단순한 형태로 바꿀 수 있다면 좋을 것 같은데, 최선이다 싶은 방법이 떠오르지는 않습니다.

int i = 0;
if (einfo->nvalue > 0) {
} else {
    i++;
}
for (; i < einfo->naddnl && rltotal > 0; i++) {

@ing-eoking ing-eoking marked this pull request as ready for review April 3, 2024 08:31
@jhpark816
Copy link
Copy Markdown
Collaborator

개인적으로 아래 코드를 더 단순한 형태로 바꿀 수 있다면 좋을 것 같은데, 최선이다 싶은 방법이 떠오르지는 않습니다.

코드를 더 간단하고 이해하기 쉬운 형태로 변경해 주면 좋겠습니다.

jhpark816

This comment was marked as resolved.

@ing-eoking ing-eoking marked this pull request as draft April 4, 2024 01:14
@ing-eoking ing-eoking closed this Apr 4, 2024
@ing-eoking ing-eoking reopened this Apr 4, 2024
@ing-eoking ing-eoking force-pushed the ritem branch 4 times, most recently from 06716ca to 3d70157 Compare April 4, 2024 03:23
jhpark816

This comment was marked as resolved.

jhpark816

This comment was marked as resolved.

@ing-eoking

This comment was marked as resolved.

Comment thread memcached.c Outdated
@ing-eoking

This comment was marked as resolved.

Copy link
Copy Markdown
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.

리뷰 완료

Comment thread memcached.c
@ing-eoking
Copy link
Copy Markdown
Collaborator Author

ing-eoking commented Oct 24, 2024

@jhpark816 @namsic
해당 작업도 조금씩 진행해야 하기에 이전 논의 내용을 재정리하겠습니다.

  1. 추가 데이터를 읽기 위한 nread에 필요한 ritem_set_first와 ritem_set_next 함수가 존재
  2. ritem_set_first는 데이터를 읽어들일 공간 중 첫 번째를 지정하는 함수로, 프로토콜 모듈에서만 사용
  3. ritem_set_next는 이전 데이터 공간이 가득 찼을 때 다음 데이터 공간으로 전환하는 함수로, core에서만 활용
  4. 이 함수들의 의미상 별도의 모듈로 분리하기 보다는 함께 두는 것이 더 적절하다고 판단
  5. 따라서, ritem_set_first와 ritem_set_next 없이도 모든 데이터에 직접 접근할 수 있는 rlist를 두고 이를 활용
  6. 프로토콜 모듈에서는 add_ritem을 통해 데이터 공간을 rlist에 담아 core에 전달하며, core는 이를 활용하여 데이터를 읽는 방식으로 작동

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.

3 participants