Aim 58 impl udp server #26

maiload · 2024-11-18T01:15:17Z

☝️Issue Number

✨ feat: Impl Udp Server #8

🔎 Key Changes

원래는 UDP 연결을 마무리지으려고 만든 브랜치였으나, 웹소켓 쪽에 이슈 발생 및 마무리할 부분이 있어 Voice, Video 웹소켓 통신을 마무리함
Redis Stream에서 발생하는 이벤트의 전달이 묻히는 이슈가 있었으나, Redis Readoffset 옵션을 변경함으로써 수정

💌 To Reviewers

UDP 연결 및 중계 로직은 다음 티켓에서 진행할 예정입니다.

src/main/java/com/example/pitching/call/handler/CallWebSocketHandler.java

src/main/java/com/example/pitching/call/operation/request/InitRequest.java

KmYgJn · 2024-11-18T02:29:20Z

src/main/java/com/example/pitching/call/server/CallUdpClient.java

+    }
+
+    private Mono<Sinks.Many<DatagramPacket>> getChannelSink(String channelId) {
+        // TODO: get 에서 에러가 발생하면 서버가 끊김


해당 TODO의 해결방식이 명시되지 않은 것 같습니다. TODO는 문제를 명시하기보다 해야할 일을 명시해주면 좋을 것 같아요!

UDP 관련된 부분은 이번 티켓에서 다루지 않을 예정이기도 하고, 해당 문제도 테스트 후 정확한 진단이 필요해서 다음 티켓에서 정확히 분석 후 진행하겠습니다.

아니요. UDP와 관련한 작업은 이후에 작업해주시면 되는 부분인 것 같고요.

TODO 주석은 이후 어떤 작업을 해야될지 나타내야하는 주석이라고 생각을 해요. develop 브랜치에 merge 되는 부분은 문제 없는 부분들이 merge 되야한다고 생각하는데, 해당 주석은 merge 되기에는 맞지 않다고 생각해서 이번 pr에서 수정해주셨으면 좋겠습니다.

KmYgJn · 2024-11-18T02:33:27Z

src/main/java/com/example/pitching/call/server/CallUdpClient.java

+                .wiretap(true)
+                .doOnChannelInit((observer, channel, remote) ->
+                        channel.pipeline().addFirst(new LoggingHandler("reactor.netty.examples")))
+                .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, 5000);


해당 5000이라는 값은 옆에 MILLIS를 보고나서야 5초라는 것을 알 수 있는 값 인 것 같아요. 의미있는 변수명을 통해 명시해주는 게 보다 좋을 것 같습니다! 또한, 유지보수 측면에서도 yml에서 값을 설정하는 것이 좋아보여요!

UDP 관련된 부분은 이번 티켓에서 다루지 않을 예정이기도 하고, 다음 티켓에서 정리해서 반영해도 될까요?

위에 적은 TODO와 같은 맥락이라고 생각합니다. 이번 브랜치에서 수정해주셨으면 좋겠습니다. 만약, 이후 브랜치에서 수정하지 않을거라면 TODO 처리해주시면 좋을 것 같아요

KmYgJn · 2024-11-18T02:37:15Z

src/main/java/com/example/pitching/call/server/CallUdpClient.java

+
+    public void initializeCallSink(ChannelEnterResponse channelEnterResponse) {
+        // Create clientSink
+        if (!clientSinkMap.containsKey(channelEnterResponse.userId())) {


여러 스레드가 동시에 containsKey를 통과할 때, 동시성 문제가 발생하는 상황은 없을까요??

UDP 관련된 부분은 이번 티켓에서 다루지 않을 예정이기도 하고, 다음 티켓에서 정리해서 반영해도 될까요?

좋습니다. TODO 작성해주세요!

KmYgJn · 2024-11-18T02:40:43Z

src/main/java/com/example/pitching/call/server/VoiceUdpServer.java

+
+    private NettyOutbound handleVoiceData(UdpInbound in, UdpOutbound out) {
+        return out.sendObject(
+                // TODO: 자신이 송출한 데이터는 되돌려 받지 않기 때문에 Void를 반환하는데, UDP 연결이 안끊기고 있음


TODO는 문제를 명시하기 보다는 해야할 일을 작성하는 것이 보다 좋아보여요! 아래처럼 작성해주시면, 어떠한 작업이 필요하다는 것을 보다 명확하게 이해할 수 있을 것 같습니다.

// 변경 전 // TODO: 자신이 송출한 데이터는 되돌려 받지 않기 때문에 Void를 반환하는데, UDP 연결이 안끊기고 있음 // 변경 후 // TODO: UDP 연결 자원을 명시적으로 해제하는 dispose() 메서드 구현 필요 // 또는 // TODO: UDP 소켓 연결 종료 후 리소스 정리하는 cleanup 로직 추가

UDP 관련된 부분은 이번 티켓에서 다루지 않을 예정이기도 하고, 다음 티켓에서 정리해서 반영해도 될까요?

해당 주석은 정확히 해야할 일을 이번 PR에서 명시해주시면 감사하겠습니다.

KmYgJn · 2024-11-18T02:44:59Z

src/main/java/com/example/pitching/call/service/ActiveUserManager.java

-    public Mono<Boolean> removeActiveUser(String userId) {
-        return valueOperations.delete(getActiveUserRedisKey(userId));
+    public Mono<String> removeUserActiveFromRedis(String userId) {
+        return valueOperations.getAndDelete(getActiveUserRedisKey(userId));


단일 책임 원칙에 어긋나는 것 같아요. get과 delete 2개의 책임을 지고 있기 때문입니다. 또한, delete의 경우 void를 반환하는 것이 관행이라고 생각하는 데 String 값이 반환되는 이유가 있을까요??

해당 메서드는 제가 정의한 메서드가 아니고, 제공되는 메서드 입니다.
기존의 있던 값을 get으로 가져오고 delete 하는 메서드입니다!
보통 삭제되기 전의 값이 필요한 경우 사용합니다.

찾아보니까 SRP보다는 원자성 보장을 위해서 제공하는 메서드인 것 같네요! 좋습니다~

KmYgJn · 2024-11-18T02:46:30Z

src/main/java/com/example/pitching/call/service/ActiveUserManager.java

-                            .flatMap(pastServerId -> validateServerDestination(serverId, pastServerId));
+                            .map(pastServerId -> validateServerDestination(serverId, pastServerId))
+                            .doOnSuccess(isChanged -> {
+                                if (isChanged) {


조건문을 filter로 대체할 수 있을 것 같은데, if문을 사용하신 이유가 있을까요??

filter를 하면 말그대로 필터링이되어서, false 라는 값이 뒤에 전달이 안되기 때문에 doOnSuccess를 사용했습니다.

return valueOperations.getAndSet(getActiveUserRedisKey(userId), serverId) .flatMap(pastServerId -> validateServerDestination(serverId, pastServerId)) .filter(isChanged -> isChanged) // true인 경우만 다음 로직 실행 .doOnNext(ignored -> log.info("ALREADY ACTIVE BUT NOT SAME SERVER ID")) .flatMap(ignored -> setSubscriptionRequired(userId, true)) .defaultIfEmpty(false); // false인 경우 기본값 처리

클로드는 위처럼 작성하면 된다고 하던데, 문제가 되는 부분이 있을까요??

오!
이 방법이 더 깔끔한거 같네요. 감사합니다.

KmYgJn · 2024-11-18T02:49:11Z

src/main/java/com/example/pitching/call/service/ServerStreamManager.java

-                disposable.dispose();
-                log.info("Unregister server stream: {}", serverId);
+        return Flux.defer(() -> {
+            synchronized (this) {


synchronized는 webflux의 non-blocking 방식과 충돌이 있을 것 같은데, 사용하신 이유가 있을까요??

동기화를 사용하면 논블로킹 방식에서 이점을 얻을 수 없지만, 해당 부분은 속도보다는 정합성을 우선시하는게 맞다고 생각하여 사용했습니다. (사용하지 않으면 프론트에서 Strict mode에 의해 엄청 짧은 시간에 반복 요청을 보내는 경우 2번 수행이 되는 이슈가 있었습니다.)

클로드는 synchronized와 비교해봤을 때 더 좋은 대안으로 computeIfAbsent를 추천하더라고요! 혹시 알아보셨을까요??!

computeIfAbsent 메서드는 단순히 값을 확인하고 새로운 값을 넣는 두 동작의 원자성만을 보장해주기 때문에 해당 객체의 접근 자체를 동기화시켜주지는 못하기 때문에 사용하지 않았습니다.

- Change using Spring Security to using first socket message for authorization

maiload added the feat 기능 추가 label Nov 18, 2024

maiload requested review from logxingit and KmYgJn November 18, 2024 01:15

maiload self-assigned this Nov 18, 2024

KmYgJn reviewed Nov 18, 2024

View reviewed changes

maiload force-pushed the AIM-58-Impl-UDP-Server branch from 2fbfcbe to c662d3b Compare November 30, 2024 17:22

maiload added 24 commits December 1, 2024 02:22

refactor: Use computeIfPresent() in HashMap

b21716e

fix: Remove ip, port fields

d4f0b36

feat: Add test user

0c3b99c

fix: Remove unused methods

76ce803

refactor: Merge clean up methods

1688268

fix: Change ignored argument to _

e9b856b

fix: Change method name

94115a2

feat: Add UdpAddressManager

72d0b6d

feat: Impl VoiceUdpServer and CallUdpClient

0c4bbfb

fix: Change log sentence

eb27a5b

refactor: Change _ to ignored

8a1fa23

chore: Add prostgresql docker compose

2d3b440

fix: Remove useless codes

c877ab6

feat: Add Security logic for webSocket

7f44a21

feat: Fix Authorization logic in WebSocket

13ea7e1

- Change using Spring Security to using first socket message for authorization

Fix: Add condition to CallUdpClient initializeCallSink

1a271e3

fix: Change log

42ff1aa

fix: Change Server Sink Oprtion to directAllOrNothing

b670641

fix: Make serverSink when user request server enter

0f28dae

Bug: Set Redis Stream ReadOffset lastest to lastConsumed

6a05d73

feat: Add User Profile Info to ChannelEnterResponse

1c1e978

feat: Add profile Image to Server Enter Response

7535a9f

feat: Add test channelId

6a3638e

refactor: Accept feedback

16ef170

maiload added 13 commits December 1, 2024 02:22

refactor: Change chaining method

f832308

fix: Change serverId type (Str -> Long)

ab29b89

fix: Change channelId type (Str -> Long)

826d89e

chore: Add Kurento dependency

3ad2542

feat: Add Kurento Clinet and KMS

94c218e

fix: Fix dispose logic of Subscription

ba32f59

fix: Remove isSpeaking, isScreenSharing field

f974c25

fix: Fix cleanupWebRtc()

2efc79c

Fix: Bug Fix in Presenter answer

709932d

fix: Delete isSpeaking, isScreenSharing fields

6ed2112

feat: Add ServerResponse and Change logs

143fa94

fix: Allow duplicated channel enter

6bc777b

fix: Add import

917d4f4

maiload force-pushed the AIM-58-Impl-UDP-Server branch from c662d3b to 917d4f4 Compare November 30, 2024 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aim 58 impl udp server #26

Aim 58 impl udp server #26

maiload commented Nov 18, 2024

KmYgJn Nov 18, 2024

maiload Nov 18, 2024

KmYgJn Nov 18, 2024

KmYgJn Nov 18, 2024

maiload Nov 18, 2024

KmYgJn Nov 18, 2024

KmYgJn Nov 18, 2024

maiload Nov 18, 2024

KmYgJn Nov 18, 2024

KmYgJn Nov 18, 2024

maiload Nov 18, 2024

KmYgJn Nov 18, 2024

KmYgJn Nov 18, 2024

maiload Nov 18, 2024

KmYgJn Nov 18, 2024

KmYgJn Nov 18, 2024

maiload Nov 18, 2024

KmYgJn Nov 18, 2024

maiload Nov 19, 2024

KmYgJn Nov 18, 2024

maiload Nov 18, 2024 •

edited

Loading

KmYgJn Nov 18, 2024

maiload Nov 19, 2024

Aim 58 impl udp server #26

Are you sure you want to change the base?

Aim 58 impl udp server #26

Conversation

maiload commented Nov 18, 2024

☝️Issue Number

🔎 Key Changes

💌 To Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiload Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiload Nov 18, 2024 •

edited

Loading