Log Service Setup #1721

weiligu · 2024-02-15T01:32:05Z

Description of changes

https://linear.app/trychroma/issue/CHR-241/stand-up-log-service

Stand up Log Service in Dev
- stand up postgres DB
- stand up migration: atlas - depend on postgres
- stand up logservice - depend on migration
- stand up coordinator - depend on migration
database migration
- change env name
- change database name
- add definition for reccord log (we can test perf for this later, not hard to change)
log service: go
- entry point: main with Cmd
- grpc service: with proto change
coordinator
- connect to docker postgres
- reorganize packages to accommodate with logservice
- rename bin to coordinator instead of chroma
- tests connect to local postgres instead of sqlite
  - fix a bug from segment delete

Test plan

How are these changes tested?

Tilt Up successfully run, docker containers successfully starts
Coordinator tests passes

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

github-actions · 2024-02-15T01:32:25Z

go/coordinator/Dockerfile.migration

go/coordinator/Makefile

HammadB · 2024-02-15T01:45:16Z

go/coordinator/cmd/coordinator/cmd.go

-	Cmd.Flags().StringVar(&conf.Username, "username", "root", "MetaTable username")
-	Cmd.Flags().StringVar(&conf.Password, "password", "", "MetaTable password")
-	Cmd.Flags().StringVar(&conf.Address, "db-address", "127.0.0.1", "MetaTable db address")
+	Cmd.Flags().StringVar(&conf.SystemCatalogProvider, "system-catalog-provider", "database", "System catalog provider")


are we sure want to change the default?

yes because memory is not Postgres and I want to have local end to end set up to include postgres

ok, can we add some tooling to clear/nuke postgres in that case as a follow up? its useful while debugging to be able to easily clear the sysdb

3 parts of this:

unit tests: should clean up the state - add setup and teardown (I can add this, very easy)

local tests: scripts to nuke, for now the easiest thing is to just drop and re-create the db

docker end to end test: there is a refresh button in tilt UI, one click should do the work

Docker restart should not remove state. We should use volumes correctly and have a script provided for People to reset the db

I do not use volume for the local docker setup as of now. can go add that later if we want persistence.

We should use volumes. Lets do the right thing from the get go please!

go/coordinator/internal/logservice/grpc/server.go

HammadB · 2024-02-15T01:47:25Z

go/coordinator/internal/metastore/db/dao/segment_metadata.go

@@ -21,7 +21,7 @@ func (s *segmentMetadataDb) DeleteBySegmentID(segmentID string) error {
 func (s *segmentMetadataDb) DeleteBySegmentIDAndKeys(segmentID string, keys []string) error {
 	return s.db.
 		Where("segment_id = ?", segmentID).
-		Where("`key` IN ?", keys).
+		Where("key IN ?", keys).


Reason for change?

because unit tests were using sqlite and this throws an error when I change them to postgres

k8s/dev/migration.yaml

HammadB · 2024-02-15T01:51:05Z

k8s/dev/logservice.yaml

@@ -0,0 +1,39 @@
+apiVersion: apps/v1


note: probably not wanted in dev/. dev/ is meant for deployments we only use while developing (like the postgres service)

maybe we should rename it, but the /dev is use by tilt only

woops! my bad - i confused this for test/ - I didn't realize we added dev/.

Should we add these to deployments/ too then?

I don't think yet we decided how to do sql migration EDIT: my comment was not related to the right file.

Yes, we should also add it in deployment.

sorry I was confused? This is already in dev and we don't want to add this to deployment because we probably will do this differently?

https://github.com/chroma-core/chroma/blob/main/k8s/deployment/kubernetes.yaml we should add the deployment for the log service in that file too.

oh I see, this is what is used for minikube and kube apply. ok I can add that. Can we make them into one version later?

HammadB

This makes sense. Please try to break up larger PRs in the future. There are several logically separate changes in this PR. I ask that we make sure @Ishiihara Has a chance to look at this before we merge, given that he's done the bulk of our Go work to date.

HammadB · 2024-02-15T01:52:38Z

Also the coordinator tests are segfaulting.

FAIL: TestCreateGetDeleteCollections (0.01s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference

https://github.com/chroma-core/chroma/actions/runs/7909839228/job/21591525871?pr=1721

go/coordinator/internal/metastore/db/dbcore/core.go

weiligu · 2024-02-15T18:12:06Z

Also the coordinator tests are segfaulting.
FAIL: TestCreateGetDeleteCollections (0.01s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
https://github.com/chroma-core/chroma/actions/runs/7909839228/job/21591525871?pr=1721

yea this is because we don't have postgres in our github setups. I will post to the team channel about this

weiligu · 2024-02-15T18:13:30Z

This makes sense. Please try to break up larger PRs in the future. There are several logically separate changes in this PR. I ask that we make sure @Ishiihara Has a chance to look at this before we merge, given that he's done the bulk of our Go work to date.

He is out until next week. Are you ok with me merging but ask him to review and make changes in separate PR next week?

HammadB · 2024-02-15T18:24:02Z

This makes sense. Please try to break up larger PRs in the future. There are several logically separate changes in this PR. I ask that we make sure @Ishiihara Has a chance to look at this before we merge, given that he's done the bulk of our Go work to date.

He is out until next week. Are you ok with me merging but ask him to review and make changes in separate PR next week?

Yeah that is fine - this was before he sent his OOO :)

beggers

If I'm understanding correctly this seems like two separate changes to me:

Initialize logservice code and stand up log service.
Move coordinator off sqlite for testing.

Could we split these into two PRs? If I'm mistaken or there's a strict requirement for these to be 1 PR please tell me.

beggers · 2024-02-16T17:23:48Z

k8s/deployment/kubernetes.yaml

@@ -188,7 +258,7 @@ spec:
    spec:
      containers:
        - command:
-            - "chroma"
+            - "coordinator"
            - "coordinator"


Nit: I feel a little weird about these commands being coordinator coordinator and logservice logservice. Could we make them coordinator run and logservice run or something like that?

yea sounds good to me I will open a follow up pr for this

beggers · 2024-02-16T17:29:21Z

go/coordinator/cmd/logservice/cmd.go

I don't like the logservice living in go/coordinator. Could we move it to go/logservice instead?

My plan is to have:

coordinator (we can rename this to sth else later)

logservice

sysdb

cluster manager

does that sounds good to you?

weiligu · 2024-02-16T18:53:20Z

If I'm understanding correctly this seems like two separate changes to me:

Initialize logservice code and stand up log service.

Move coordinator off sqlite for testing.

Could we split these into two PRs? If I'm mistaken or there's a strict requirement for these to be 1 PR please tell me.

Yes they don't have to come in one PR. The logservice shares the same code connecting to Postgres so I made the change in one PR. Will separate them next time!

## Description of changes https://linear.app/trychroma/issue/CHR-241/stand-up-log-service - Stand up Log Service in Dev - stand up postgres DB - stand up migration: atlas - depend on postgres - stand up logservice - depend on migration - stand up coordinator - depend on migration - database migration - change env name - change database name - add definition for reccord log (we can test perf for this later, not hard to change) - log service: go - entry point: main with Cmd - grpc service: with proto change - coordinator - connect to docker postgres - reorganize packages to accommodate with logservice - rename bin to coordinator instead of chroma - tests connect to local postgres instead of sqlite - fix a bug from segment delete - system_test fix will be in a separate PR

Weili Gu added 4 commits February 14, 2024 16:35

stand up log service

343efbf

fix lib naming

02a3b0d

revert segment schema change, do it later

80adbd3

fix segment delete

83057e5

weiligu requested review from Ishiihara, nicolasgere and beggers February 15, 2024 01:32