-
Notifications
You must be signed in to change notification settings - Fork 0
[feature] 픽 실시간 업데이트 #174
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
[feature] 픽 실시간 업데이트 #174
Conversation
@miller198 style 커밋 때문에 알려주신 것 같은데 알려주셔서 감사합니다! 이 값이 다르면 서로 코드 자동 정렬하는 게 다를 것 같아 이 부분도 프로젝트 내에서 통일하면 좋을 것 같네요 |
저는 반대로 200으로 쓰고있었네요! ㅋㅋ 130으로 맞추는 것 괜찮은 것 같습니당 |
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.
실시간으로 화면에 픽이 생성되고 삭제되는 기능 잘 동작함을 확인했습니다 😄 (테스트도 재밌네요 ㅎㅎ)
-
Repository와 DataSource에서 타입 바꾸신 것 말씀하신대로 역할이 분명히 분리되어 있다면 괜찮다고 생각합니다. 그리고 사용하지 않는 Exception 삭제해도 될 것 같아요!
-
마커 클릭 해제에 대하여 만약 개선하고 싶다면 클러스터 상태가 변경될 때 콜백을 실행하는 방식으로 구현할 수 있을 것 같습니다.
하지만 만약 상태 변경이 계속 일어나서 사용자가 특별한 조작을 하지 않았음에도 단말클릭상태 <-> 클러스터 선택 상태가 반복해서 변경된다면 이 또한 사용자에게 더 큰 혼동을 줄 수 있을 것이라 생각합니다. (클릭하려고 하는데 자꾸 바뀌는 등의 혼란이 있을 수 있을 것 같아요 @_@)
그래서 개인적인 의견으로는 이 부분은 오히려 클릭한 상태일 때는 사용자가 추가적인 동작을 했을 때 변경되도록 두는 것도 괜찮다고 생각합니다.
- Mutex 처리해주신 것 좋습니다!
val nearPickSet = mutableSetOf<Pick>() | ||
fetchPickUseCase(location.latitude, location.longitude, notiRadius) | ||
.onSuccess { | ||
_nearPicks.emit(it) | ||
}.onFailure { | ||
_nearPicks.emit(emptyList()) | ||
.collect { pickList -> | ||
nearPickSet.addAll(pickList) | ||
_nearPicks.emit(nearPickSet.toList()) |
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.
여기는 pick의 중복방지를 위해 set을 사용하신 것 같은데, 중복으로 불러와지는 경우가 어떨 때 발생하나요?
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.
그러게요 ㅎㅎ..? 저기에서 set을 사용할 이유가 없어보이네요 삭제했습니다!
제가 처음에는 repository에서 해당 영역에 존재하는 리스트를 반환하는 게 아니라 추가되는 픽들만 반환하는 식으로 구상했어서 그랬던 것 같네용
queries.add(query) | ||
} | ||
|
||
): Flow<List<Pair<PickType, Pick>>> = callbackFlow { |
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.
Pair나 Triple 같은 타입은 구현하는 사람 입장에선 이해가 쉽지만 다른 개발자 입장에선 이해가 힘들 수 있어 사용을 자제하라는 말을 들었어요
찾아보니 이 글에서 비슷한 내용이 언급되어있네요
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.
저도 Pair는 .first, .second로 값에 접근하기 때문에 유지보수 측면에서 어려울 수 있어 사용을 자제하라는 말은 많이 들어서 알고 있었습니다. 그래서 평소에 Triple은 전혀 사용하지 않고 Pair도 잘 사용하지 않는데 그럼에도 Pair를 사용했던 이유는 코드에서 .first, .second로 값에 접근하지 않았고, 타입이 PickType, Pick이라는 것에서 이미 무슨 값인지 잘 알 수 있겠지? 라는 생각 때문이었습니다.
구조 분해를 사용하여 first, second가 아닌 type, pick으로 값에 접근했었습니다.
firebaseRemoteDataSource.fetchPicksInArea(lat, lng, radiusInM)
.map { pickList ->
pickList.forEach { (type, pick) ->
}
}
그런데 생각해보니 이건 코드를 작성한 제 입장이고 다른 사람들이 읽었을 땐 아닐 수 있다는 걸 다시 한 번 머리에 넣어두겠습니다. 언급해주신 글을 보고 Pair나 Triple이 lazy data class라는 건 몰랐는데 알아갑니다!
Pair를 사용하던 것에서 PickWithType이라는 data class를 만들어서 수정했는데 어떤가요?
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.
좋습니다!!
} | ||
trySend(pickData).isSuccess |
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.
isSuccess가 필요한가요?
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.
음 아니요.. 필요없을 것 같습니다
} | ||
} catch (e: Exception) { | ||
close(e) |
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.
여기나 위의 close(e)가 호출되면 이 callbackFlow를 받는 repository 코드에서 catch 로 예외 e에 대한 처리를 해야하지않을까요?
@@ -13,7 +19,14 @@ class FirebaseRepositoryImpl @Inject constructor( | |||
private val firebaseRemoteDataSource: FirebaseRemoteDataSource | |||
) : FirebaseRepository { | |||
|
|||
override suspend fun createGoogleIdUser(uid: String, email: String, userName: String?, userProfileImage: String?): Result<User> { | |||
private val latestNearPickMutex = Mutex() |
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.
혹시 이 뮤텍스가 왜 필요한지 자세히 설명해주실수있나요
동시성 문제가 언제 발생하는지 잘 이해가 안되서요 ㅠ
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.
으음 제가 PR을 올리며 작성한 글에도 Mutex가 필요한 게 확실한 건지 모르겠다는 말을 쓰긴 했지만 ㅎ.. 제가 동시성 문제가 발생할 거라고 생각한 경우는 아래와 같습니다
픽이 어떤 기준으로 불러와지는지는 알고 계실 거라 생각해요 참고
현재 구현된 코드에서는 Geohash 필드마다 flow가 하나씩 존재합니다.
예를 들어서 현재 보고 있는 화면의 지도 영역이 위와 같다고 하면 Geohash가 ezz, u00 외에도 몇 개가 있을 겁니다. 사용자 A와 B가 각각 ezz, u00에서 픽을 하나씩 동시에 추가한다고 하면 ezz flow에서 값(새로 추가된 픽)을 방출하고, u00 flow에서도 값을 방출합니다. 만약 뮤텍스를 사용하지 않으면 두 flow에서 기존 리스트 + 새로 추가된 픽을 따로 업데이트하여 latestNearPick이 제대로 된 최신 주변 픽 리스트가 아닐 거라 생각했습니다. 기존 리스트가 30개라고 해봅시다. 보고 있는 화면에서 2개가 추가되면 리스트가 32개여야 할 겁니다. 뮤텍스가 없으면 두 flow에서 기존의 리스트에 동시에 접근할 수도 있다고 생각합니다. 그렇게 되면 아래처럼 될 거라 생각했습니다
- ezz flow : 기존 리스트(30개)에 ezz 영역에 새로 추가된 픽이 추가된 리스트 -> 31개
- u00 flow : 기존 리스트(30개)에 u00 영역에 새로 추가된 픽이 추가된 리스트 -> 31개
뮤텍스를 사용한다면 여러 개의 flow가 동시에 latestNearPick에 접근하지 않게 되어 30개에서 32개로 안전하게 업데이트될 것이라고 생각했습니다.
제가 필요한 게 확실한 건지 모르겠다고 한 이유는 이런 상황을 직접 보고 확인해보지는 못해서ㅜ입니다.. 혹시 제 생각이 틀린 것 같다면 알려주세요!
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.
앗 리뷰한지가 오래되었다가 다시 코드를 보니 mutex가 필요없는 것 같다고 생각합니다..!
현재 FirebaseRepositoryImpl
파일의 fetchPicksInArea
에서 사용되는 latestNearPick
이 지역 변수로 선언되어 있습니다.
여러 개의 fetchPicksInArea
에서 동시에 호출을 해도, 각 코루틴이 별개의 latestNearPick
을 사용하기 때문에 동시에 접근하지 않을 것 같아요!
제가 생각했던건 MapViewModel
파일의 fetchPicksInBounds
내부의 mutex 필요성인데 여기서는 _picks나 clusterer를 업데이트할 때 민주님이 답변해주신대로 여러개의 값을 업데이트되는 문제를 방지하기 위해 필요하다고 생각했었습니다.
그런데, 현재 fetchPicksInBounds
를 호출하는 경우는 디바이스를 초기설정하는 경우와 LaunchedEffect
내부에 있습니다.
공식 문서에서 LaunchedEffect의 키가 변경되면 기존 코루틴이 취소되고 새 코루틴에서 실행된다는 내용이 있네요..!
fetchPicksInBounds
가 동시에 실행되는 경우가 없다면 mutex가 필요없지 않을까 라는 생각이 들었습니다.
직접 로그를 찍어보아도 각 코루틴이 종료되고 실행됨을 확인했습니다.
혹시 위 내용도 이상한 점이 있다면 말씀해주시면 감사하겠습니다!
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.
지난 회의 이후 깨달았습니다! 뮤텍스는 필요 없는 것 같네요 반영해두었습니다
저는 데이터 소스의 fetchPicksInArea()
에서 여러 개의 Flow를 방출하는 줄 알았습니다 ㅎㅎ... trySend()
로 한 channel에 데이터를 계속해서 보내는 거라 결국은 하나의 Flow에서 계속해서 데이터를 방출하는 거였네요
Co-authored-by: Ju YunGyeom <[email protected]>
지난 회의에서 Flow가 종료되면 이걸 처리하는 데가 없어서 앱이 꺼지는 문제가 있다고 이야기해주신 부분 처리해서 올려뒀습니다. 일반적으로 테스트할 때는 try-catch 구문에서 catch로 빠지는 경우가 없어서 close(e)가 되었을 때의 처리를 잘 해두지는 않았던 것 같습니다. 시뮬레이션으로는 어떻게 해야 catch 구문으로 빠지는지 잘 모르겠어서 코드 상 일부러 오류를 발생시켜 테스트하긴 했습니다. 예외 처리를 Screen_recording_20250407_032539.webm |
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.
좋습니다!! 😁
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.
고생하셨어요! 주석이나 로그 정리 + develop 머지 한번 해주세요!!
#️⃣연관된 이슈
📝작업 내용 및 코드
결과
다른 사용자가 새로운 픽을 추가/삭제했을 때 이를 지도에서 보려면 지도를 움직여야 했던 것을 실시간으로 지도 화면에서 반영되도록 했습니다.
기존
B는 A의 현재 위치를 포함한 지도 화면을 보고 있습니다. B는 지도에서 아무것도 하지 않고 있을 때 A가 현재 위치에 픽을 등록해도 B는 바로 업데이트 받을 수 없습니다. B는 지도 화면을 움직이거나 다른 화면으로 이동하고 돌아온 후에야 A가 등록한 픽을 확인할 수 있습니다.
현재
1.mp4
과정
addSnapshotListener에 대한 검토
참고) 공식 문서
실시간 업데이트를 위해
query.get()
을 사용하던 것을query.addSnapshotListener()
를 사용해도 될 지에 대해 공식 문서를 보며 해당 리스너에 대해 알아보고 검토했다.현재 프로젝트에서는 픽 데이터 하나를 Firestore에서 picks 컬렉션의 문서 하나로 하여 진행하고 있다. 따라서 픽 리스트 실시간 업데이트라 하면 컬렉션에서 문서의 추가 및 삭제가 감지될 수 있는지를 알아야 했다.
사실 처음에는 공식 문서의 첫 문장이 “You can listen to a document with the onSnapshot() method.” 라고 되어 있어 안 되는 줄 알았다. (ㅎㅎ;) 문서를 읽다 보니 아래에 컬렉션에서 문서를 listen 할 수 있는 방법에 대해 나와있어 가능하다는 것을 파악했다.
다음으로 알아본 건 최초의 데이터는 어떻게 되는 건지였다. 실시간 변경에 대한 기준 데이터(초기 상태)를 만들어줘야 하는지와 같이 따로 어떤 처리가 필요한지 알아야 했다. 역시나 문서에서도 Important로 나와있는 내용이었다.
첫 번째 쿼리 스냅샷은 쿼리와 일치하는 모든 존재하는 문서에 대한 ADDED 이벤트를 포함한다고 되어 있다. 처음에는 쿼리와 일치하는 모든 문서를 추가로 보고 이걸 기준으로 변하는 데이터를 보는 것이다. 따라서 초기 상태를 따로 처리하는 과정은 필요 없다.
이렇게 검토를 마치고 DataSource에서 픽 데이터를 가져오는 부분 중
query.get()
을query.addSnapshotListener()
로 바꾸는 작업을 시작했다.fetchPicksInArea()
수정기존의 fetchPicksInArea() 함수 반환 타입(List)을 유지하려고 한다면 데이터 변경이 있을 때마다 matchingPicks를 return 해야 할 것인데 이는 너무 비효율적인 방식이라 생각했다. 그리고 찾아보니 안드로이드 공식 문서 - 데이터 레이어 내용 중 시간에 따른 데이터 변경을 감지하려면 data layer에서 flow를 노출하라고 되어 있었다.
실시간 픽 업데이트를 위해 FirebaseRepository와 FirebaseRemoteDataSource의 반환 타입을 Flow로 변경했다.
Result<List<Pick>>
→Flow<List<Pick>>
List<Pick>
→Flow<List<Pair<PickType, Pick>>>
여태 작성한 코드들을 보면 Repository와 DataSource에서 이름이 같은 함수의 반환 타입은 Result로 감싸진 차이만 있지 타입은 같았다. 하지만 이번에 픽 실시간 업데이트를 진행하며 Repository와 DataSource의
fetchPicksInArea()
함수가 Flow에서 방출하는 타입을 다르게 설정했다.두 타입을 같게 하려면
Flow<List<Pick>>
으로 같게 할 수는 있지만 다르게 한 이유는 이렇게 하면 DataSource가 너무 많은 책임을 맡게 되고 Repository가 무의미하다고 생각했기 때문이다.FirebaseRepository와 FirebaseRemoteDataSource의
fetchPicksInArea()
가 하는 일은 Firebase에서 데이터를 받고 UI에서 사용하기 위해 현재 화면에 있는 픽 데이터를 리스트로 만들어서 내보내주는 것이다. 만약 두 함수의 반환 타입을 같게 하려면 이 모든 과정을 FirebaseRemoteDataSource에서 진행해야 하고 FirebaseRepository는 그저 DataSource를 감싸는 것 밖에 안 된다.만약 DataSource에 모든 게 있다면 최신 근처 픽 데이터도 여기에서 관리해야 하는데 DataSource에 데이터를 유지시키는 건 별로라고 생각했다.
따라서 data source는 firestore의 데이터를 올려보내는 용, repository는 data source에서 받은 데이터를 처리하는 쪽으로 하고 둘의 반환 타입을 다르게 했다. repository에서 픽 리스트를 관리하기 위해서는 받은 픽이 삭제된 건지 새로 생긴 건지를 알아야 하기 때문에 data source에서 보내는 데이터에는 픽뿐만 아니라 그 픽이 어떤 타입(UPDATED, REMOVED)인지도 포함되어야 한다. repository에서 보내는 데이터에는 최신 주변 픽 리스트면 되기 때문에 타입이 필요 없다.
기존 함수
변경 후 함수
기존의 함수와 구조는 크게 달라진 게 없다. 기존에는
query.get()
를 사용했는데query.addSnapshotListener()
를 사용하여 데이터가 변경될 때마다 알 수 있도록 했다.기존 함수
변경 후 함수
기존에는 data source에서 반환된 리스트가 비어있으면 NoSuchPickInRadiusException을 발생시키고 그렇지 않으면 data source에서 반환된 리스트를 반환했다.
변경 후에는 일단 data source에서 반환된 리스트의 요소 타입이 Pair<PickType, Pick>로 바뀌었다. 그리고 repository에서 최신 근처 픽 데이터를 관리하도록 했다. 최신 근처 픽 데이터를 관리하는 타입이 맵인 것이 리스트인 것보다 삭제할 때 효율적이라고 생각하여 맵으로 설정했다. data source에서 온 데이터에서 PickType에 따라 새로 생긴 픽(UPDATED)이라면 최신 근처 픽 데이터에 추가하고 삭제된 픽(REMOVED)이라면 최신 근처 픽 데이터에서 삭제했다. 마지막에는 최신 근처 픽 데이터 리스트를 반환한다. Mutex가 필요한 게 확실한지는 모르겠지만 그래도 혹시 몰라 Mutex를 사용해두어 동시에 여러 코루틴이 latestNearPick을 수정하는 것을 막아두었다.
마주했던 문제 1
오른쪽 사용자가 어떤 클러스터 마커의 리스트를 보고 있을 때 왼쪽 사용자가 오른쪽 사용자가 보고 있는 클러스터 마커에 포함되는 픽을 추가했을 때, 오른쪽 사용자의 클러스터 마커에 표시된 숫자는 변하지만 리스트에는 추가한 픽이 보이지 않는 문제가 있었다.
기존에는 마커 상태 업데이트를 뷰모델의 함수에서 했었는데 이를 Clusterer 쪽으로 옮겼더니 해결되었다.
기존에는
fetchPickUseCase()
로 픽 리스트를 가져온 후 마커 상태 업데이트를 뷰모델의 함수 안에서 했다.뷰모델에서 마커 상태를 관리하는 값 중 prevClickedMarker가 있는데 이는 이전에 클릭된 마커로 현재 클릭되어 있는 마커이다. 클러스터러 내부에서 만약 마커의 위치가 현재 클릭되어 있는 마커와 같다면 마커 상태를 업데이트하도록 했다.
💬리뷰 요구사항 및 참고 사항
설정을 뭔가 잘못 건드렸는지 세 번째 커밋 메시지가 깨졌네요 ;.. 작성했던 커밋 메시지는
내 주변 반경 픽 리스트 업데이트
입니다.아직 주석이나 로그가 일부 정리되지 않았는데 참고 부탁드립니다.
Repository와 DataSource의 반환 타입이나 역할에 대해 제가 위에 써둔 게 있는데 다른 분들은 어떻게 생각하시는지 궁금합니다.
Flow로 변경하며 NoSuchPickInRadiusException을 사용하지 않게 되었는데 삭제해도 될까요?
단말 마커를 누르고 있는 상태에서 그 위치에 픽이 추가되어 클러스터 마커가 되면 클릭이 해제됩니다. 이에 대해 어떻게 생각하시나용? 사실 이 경우에 마커는 클러스터 마커가 되고 인포윈도우가 떠있게 하고 싶었는데 어떻게 해야할 지 잘 모르겠습니다.

이전에 마커를 눌러보고 화면을 이동하고 이것저것 했을 때 아주 가끔 마커가 클릭되지 않는 문제가 있었는데 어느 상황에서 이런 문제가 발생했었는지 정확히 기억이 나지 않습니다. 고쳤다고 생각하는데 혹시 여전히 이런 현상이 있다면 언제 발생하는지 알려주세요!
3월 3일 월요일 회의에서 제가 공유했던 문제 중 일부(삭제 관련)를 수정했는데 여전히 다른 문제들이 있을 수 있습니다. 혹시 발견하신다면 알려주시면 감사하겠습니다!
기타
이건 제가 픽을 등록하며 본 건데
a
를 검색했을 때 가장 위에 검색되는 게 달라달라이고 그 아래 것들도 뭔가a
를 검색했을 때 이게 나오는 게 맞는 건가? 싶은 것들이 있습니다. 뭔가 확인해보면 좋을 것 같아 여기에 작성합니다.a
검색 시a
검색 시