Skip to content

Conversation

@Parkjyun
Copy link
Contributor

@Parkjyun Parkjyun commented Apr 25, 2025

Related Issue 📌

close #5

Description ✔️

  • 이미지 api를 따로 만들어 미사용 s3 object들이 많아졌습니다. 이를 삭제하는 스케줄러 도입했습니다.
  • localstack을 도입하여 local 환경에서 localstack s3를 사용하도록 변경했습니다.

To Reviewers

local과 test 환경에서 application context 로드시에 localstack을 실행하도록 설정하여 앞으로 docker 함께 켜줘야 합니다!

@Parkjyun Parkjyun requested a review from Ho-Tea April 25, 2025 22:22
@github-actions
Copy link

github-actions bot commented Apr 25, 2025

Test Results

28 files  28 suites   4s ⏱️
61 tests 61 ✅ 0 💤 0 ❌
62 runs  62 ✅ 0 💤 0 ❌

Results for commit d979767.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Apr 25, 2025

🌻Test Coverage Report

Overall Project 48.96% -2.19%
Files changed 58.35%

File Coverage
LocalStackConfig.java 100% 🍏
ImageService.java 89.29% -10.71% 🍏
MemberRepositoryAdapter.java 78.57% 🍏
S3ImageReader.java 65.79% -34.21%
S3ImageUploader.java 64.84% -35.16%
S3ImageDeleter.java 60.64% -39.36%
GlobalExceptionHandler.java 18.37% -5.65%
S3CleanUpScheduler.java 0%
ImageStorageException.java 0%
S3Config.java 0% -40.74%

Copy link
Collaborator

@Ho-Tea Ho-Tea left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!!! 확인이 늦었네요 🥲
Retry와 관련된 코멘트 남겨드렸으니 확인 부탁드려요!!!

Comment on lines 22 to 24
} catch (IOException e) {
throw new BadGatewayException("이미지 업로드 중에 오류가 발생했습니다.");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

전역 핸들러에서 BadGatewayException을 잡고있는 부분이 없기에, 아래와 같이 새롭게 정의한 ImageStorageException 으로 처리해도 괜찮을 것 같습니다!
전역 예외 핸들러에도 해당 예외를 추가해주세요!

} catch (IOException e) {
            throw new ImageStorageException("이미지 업로드 중에 오류가 발생했습니다.");
        }


@Override
public List<String> getAllKeys() {
//여기도 retrey붙여야 되나? 어차피 실패하면 내일 해버리면 떙 아닌감..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Retry를 여러번 수행하였지만 실패하는 경우는 한번 네트워크 지연으로 실패하는 경우 보다 더 심각한 경우일 것 같아용. 구분하기 위해서 Retry를 도입하는 것도 괜찮을 것 같아요!

생각해보니 이미지 업로드하는 부분에도 Retry를 안붙였었네용

    @Bean
    public S3Client getS3Client() {
        return S3Client.builder()
                .region(getRegion())
                .credentialsProvider(systemPropertyCredentialsProvider())
                .overrideConfiguration(
                        ClientOverrideConfiguration.builder()
                                .retryPolicy(RetryPolicy.defaultRetryPolicy()) // 기본 Retry 정책 적용
                                .build()
                )
                .build();
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

여기서 구분한다는게 retry 여러번 후 실패, 1회 후 실패를 구분하자는 말씀이신가용??

Copy link
Collaborator

Choose a reason for hiding this comment

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

1회 실패 후 재시도 했을 때 성공이 되는 경우와 3번 모두 실패하는 장애상황을 구분하고자 했습니다!
아래와 같이 logback에 설정을 추가하면 로깅이 남는다고는 하는데.. 상세로깅 남기는 부분은 더 알아봐야할것같아요

<logger name="software.amazon.awssdk.core.retry" level="DEBUG"/>
<logger name="software.amazon.awssdk.core.internal.http.pipeline.stages.RetryableStage" level="DEBUG"/>

Copy link
Collaborator

Choose a reason for hiding this comment

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

저희 logback-spring.xml에 아래로 변경하면 될 것 같아요

<springProfile name="dev">
    <include resource="console-appender.xml"/>
    <include resource="error-appender.xml"/>
    <include resource="warn-appender.xml"/>
    <include resource="info-appender.xml"/>

    <!-- Retry 로그 레벨 INFO로 설정 -->
    <logger name="software.amazon.awssdk.core.retry" level="INFO" additivity="false">
        <appender-ref ref="STDOUT"/>
        <appender-ref ref="INFO"/>
    </logger>

    <logger name="software.amazon.awssdk.core.internal.http.pipeline.stages.RetryableStage" level="INFO" additivity="false">
        <appender-ref ref="STDOUT"/>
        <appender-ref ref="INFO"/>
    </logger>

    <!-- 루트 로거 통합 -->
    <root level="info">
        <appender-ref ref="STDOUT"/>
        <appender-ref ref="INFO"/>
        <appender-ref ref="ERROR"/>
        <appender-ref ref="WARN"/>
    </root>
</springProfile>

.build();

RequestBody requestBody = RequestBody.fromBytes(image.getBytes());
s3Client.putObject(request, requestBody);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Retry로직이 S3Client에 적용된다면 아래와 같은 후속작업이 있어야 할 것 같슴다

try {
    s3Client.putObject(request, requestBody);
} catch (S3Exception | SdkClientException e) {
    // 이 블록에 들어온 순간 모든 자동 재시도를 다 했지만 실패했다는 의미
    log.error("S3 업로드 실패: 모든 재시도 실패", e);
    throw e;
}

Copy link
Contributor Author

@Parkjyun Parkjyun Apr 29, 2025

Choose a reason for hiding this comment

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

재시도 모두 실패 시 네트워크 오류인지, 서비스 문제(권한, 버킷 존재 유무)인지 구분해서 로깅하도록 반영했습니다!
s3sdk에서 알아서 재시도 할만한 오류인지 판별해서 재시도를 해주는데..
사진 올리기에도 재시도 여부에 따른 응답/로깅 구분이 필요할까요?

Copy link
Contributor Author

@Parkjyun Parkjyun left a comment

Choose a reason for hiding this comment

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

리뷰 반영했습니다.
개별 코멘트에도 달아놓은 궁금증이긴 한데 retry여부 구분에 대한 의견이 궁금합니당.

또 timeout을 일단 개별은 3초,
전체 timeout은 retry수 + backoff(exponential, jitter)를 고려해서 설정했는데,, 적절한지.. timeout어떻게 설정합니꽈..

.build();

RequestBody requestBody = RequestBody.fromBytes(image.getBytes());
s3Client.putObject(request, requestBody);
Copy link
Contributor Author

@Parkjyun Parkjyun Apr 29, 2025

Choose a reason for hiding this comment

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

재시도 모두 실패 시 네트워크 오류인지, 서비스 문제(권한, 버킷 존재 유무)인지 구분해서 로깅하도록 반영했습니다!
s3sdk에서 알아서 재시도 할만한 오류인지 판별해서 재시도를 해주는데..
사진 올리기에도 재시도 여부에 따른 응답/로깅 구분이 필요할까요?


@Override
public List<String> getAllKeys() {
//여기도 retrey붙여야 되나? 어차피 실패하면 내일 해버리면 떙 아닌감..
Copy link
Contributor Author

Choose a reason for hiding this comment

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

여기서 구분한다는게 retry 여러번 후 실패, 1회 후 실패를 구분하자는 말씀이신가용??

@Parkjyun Parkjyun changed the title feat: 마이페이지 추가작업 feat: 미사용 이미지 제거 기능 May 6, 2025
@Parkjyun Parkjyun changed the title feat: 미사용 이미지 제거 기능 feat: 미사용 s3 객체 제거 스케쥴러 May 6, 2025
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.

feat: 마이페이지 추가 작업

3 participants