Database operations for LogItems
Depends on D3161
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/backup/docker-server/contents/server/src/DatabaseManager.cpp | ||
|---|---|---|
| 144 | We're not fetching here by primary key but by partition key... but in fact it doesn't matter, as we're interested in all the logs that are associated with a given backup, so we should explicitly use backup column name here. | |
| services/backup/docker-server/contents/server/src/DatabaseManager.cpp | ||
|---|---|---|
| 144 | This is fixed in https://phabricator.ashoat.com/D3177 In order to make it work properly, I had to refactor more files/places so I decided to make it right in another diff. | |
Comment Actions
| services/backup/docker-server/contents/server/src/DatabaseManager.cpp | ||
|---|---|---|
| 144 | I realize it is easier for you as a diff author to add a new diff with fixes instead of updating your original diffs, but it's hard for your reviewers this way. Keep in mind that this approach has mental cost for each individual reviewer who has to read your code.
| |
Comment Actions
Back to your queue as I responded in D3160 and there were no doubts about the code here.
| services/backup/docker-server/contents/server/src/DatabaseManager.cpp | ||
|---|---|---|
| 144 | I wasn't looking at what's easier for me to be honest. My goal was to make my changes as clean as possible. The error with the primary key has been in services for some time now. It's basically a logic/terminology error because a primary key consists of a partition key and a sort key. Until D3177 and D3179, I treated partition keys as primary keys. It appeared now as we used primary keys that also include sort keys for the first time. The current revision adds database operations for logs after I did the same for backup items. I try to keep changes I introduce as separated as possible - isn't it the main goal of diffs flow? I didn't want to mix introducing database operations for separate items with fixing the logic error that exists in all services.
You don't have to read any later diff to understand this one I think(maybe I'm wrong). Tomek just pointed out my mistake which is going to be fixed in later diffs but the code in this revision is consistent with the rest of the codebase at this point in the timeline.
Didn't feel like it was needed as this code stands for itself I believe. Maybe I'm missing something but if I had to do these changes again, I'd use the same order. I still think it's pretty logical and has no "future connections". | |
Comment Actions
I think the best thing to do would've been to put the sort key diffs as early in the stack as possible. Think of it this way: if the sort key diffs are earlier, then we never see any code using the "old way" for primary keys, so your reviewer never has to get confused or ask a question. Whereas if you put the sort key diff later in the stack, then there are some diffs still using the "old way", which is confusing to see as a reviewer.
Basically, my point is that once you realized that the sort key needed you change, you should've interactive-rebased to the front of your stack and put the new diff there, and then rebased all of the rest of your diffs on the change. It's more work but it's easier to review.
Anyways, the underlying concerns in D3160 have been addressing so unblocking here! We can follow up about the sort key in D3177/D3179.