-
Notifications
You must be signed in to change notification settings - Fork 167
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
Download remote bucket sheet instead of GetSheets from node #6907
Conversation
ipdae
commented
Feb 26, 2025
- resolve 게임 실행시 최신 시트 데이터 다운로드를 노드대신 S3등의 별도 서빙을 통해 받고 저장하도록 처리 #6902
- 이 코드가 동작하기위해선 PlanetId별로 시트 데이터를 직접 서빙해야합니다.
- 지정된 버킷/PlanetId 경로에 있는 시트 데이터를 다운 받아서 저장시키고 노드에서 GetSheets를 호출하는대신 해당 시트를 사용하도록 처리합니다.
var dict = await Agent.GetSheetsAsync(map.Keys); | ||
sw.Stop(); | ||
|
||
// Log the elapsed time for getting state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~Get sheet data asynchronously
~Log the elapsed time fo
같이 뻔한 주석은 제거하는게 좋을 것 같습니다. (오히려 가독성이 안좋아지는 느낌입니다)
기존 주석이 제거되었는데 (// NOTE: pair.Value
is null
when the chain not contains the pair.Key
.) 체인에 데이터가 없는 경우 null로 덮혀씌워질 수 있다는 내용을 제거하는게 적절할지 생각해봐야 할 것 같습니다
아래 ~Convert dict to csv using mapping, 주석이 달림으로써 dict를 csv데이터 형식의 스트링 묶음으로 바꾸는듯한 느낌이 들어 주석을 제거하거나 csv변수명의 이름을 csvDict등으로 수정하는게 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실제론 체인에선 삭제하는 기능이 없어서 해당 주석을 삭제했는데 남겨두겠습니다.(디버깅할땐 발생할수있으니)
foreach (var sheetName in sheetNames) | ||
{ | ||
var csvName = $"{sheetName}.csv"; | ||
var sheetUrl = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
시트중에 몇십메가짜리도 있는 걸로 아는데 이렇게 되면 모든 시트에 대해 접속할 때 마다 다운로드 요청 되는 것 아닐까요? 데일리때 이야기 했던 것처럼 hash가 담긴 데이터 목록을 하나 받아서 로컬에 저장된 파일들과 비교해서 필요한 것만 받는게 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그쪽은 별도 작업이 필요해서 따로 이슈를 만들어놨습니다. planetarium/NineChronicles.RPC.Shared#39 해당 인터페이스 작업이후 작업할 예정입니다.
{ | ||
var csvName = $"{sheetName}.csv"; | ||
var sheetUrl = | ||
$"https://9c-cluster-config.s3.us-east-2.amazonaws.com/{planet}/{csvName}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_commandLineOptions.SheetBuckUrl 해당주소로 받아야 하는지 체크부탁드립니다~!