Skip to content
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

CBG-3727: Diagnostic API: Get all doc channels #6701

Merged
merged 12 commits into from
Feb 27, 2024
Merged

Conversation

mohammed-madi
Copy link
Contributor

CBG-3727

Describe your PR here...

  • Add an endpoint to get all doc channels with the spans of sequences the doc was in those channels.
    Example response:

{"CHAN1":["6-0","1-2","3-5"],"CHAN2":["4-5","2-3"],"CHAN3":["5-6"]}

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

  • Link upstream PRs
  • Update Go module dependencies when merged

Integration Tests

Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

Looks good, suggestion to avoid iterating over history more than once.

rest/diagnostic_doc_api.go Outdated Show resolved Hide resolved
rest/diagnostic_doc_api.go Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Feb 23, 2024

Redocly previews

- $ref: ../../components/parameters.yaml#/db
- $ref: ../../components/parameters.yaml#/user-name
get:
summary: Get all channels for a user
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
summary: Get all channels for a user
summary: Get channel history for a document

rest/diagnostic_doc_api.go Outdated Show resolved Hide resolved
rest/diagnostic_doc_api.go Outdated Show resolved Hide resolved
@bbrks bbrks assigned mohammed-madi and unassigned bbrks Feb 26, 2024
@mohammed-madi mohammed-madi assigned bbrks and unassigned mohammed-madi Feb 26, 2024
@bbrks
Copy link
Member

bbrks commented Feb 26, 2024

Another comment about the ChannelSetHistory data I just thought of:

Those sequence ranges can over-represent the sequences at which the docs were in the channel, and the API format doesn't distinuqish complete entries and "merged" history entries.

Might be worth formatting the history differently so we can tell which entries are complete and which are multiple compressed/compacted sequence ranges.

E.g.

  • 2-7, 8-11 represent all sequences between 2 to 7 and 8 to 11
  • 15~24 represents some sequences between 15 and 24, but not all sequences (because we compacted away the full history)

This might require an additional field in the struct containing start/end seq to denote a compacted range.

@bbrks bbrks assigned mohammed-madi and unassigned bbrks Feb 26, 2024
@mohammed-madi mohammed-madi assigned bbrks and unassigned mohammed-madi Feb 26, 2024
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

Your change to MarshalJSON isn't symmetrical with UnmarshalJSON, and will make things start to error with compacted sequence ranges.

Comment on lines +70 to +72
if pair.Compacted {
stringPair = fmt.Sprintf("%d~%d", pair.StartSeq, pair.EndSeq)
} else {
Copy link
Member

Choose a reason for hiding this comment

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

If we're doing this text representation in MarshalJSON, an equivalent should be put in UnmarshalJSON.

Right now the two aren't compatible, and will error when running the splitPair := strings.Split(stringPair, "-") step.

@bbrks bbrks merged commit 70a591b into master Feb 27, 2024
33 checks passed
@bbrks bbrks deleted the CBG-3727_getDocChannels branch February 27, 2024 12:05
bbrks pushed a commit that referenced this pull request Mar 28, 2024
* Add handling and test for get all doc channels

* Fix lint

* Add licensing

* Address comments

* Add docs

* fix yamllint

* Add DocumentHistoryMaxEntriesPerChannel test case and fix docs

* Denote compacted sequence pairs with ~

* Fix goimports

* Fix comment

* Fix comment

* Add unmarshalJSON handlign for compacted sequence pair
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.

2 participants