-
Notifications
You must be signed in to change notification settings - Fork 14
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
server: Add bookmarks endpoint support #102
server: Add bookmarks endpoint support #102
Conversation
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.
First review. I haven't tested the functionality, I only tried to generate the openapi.yaml. The resulting file has errors and Swagger preview in vscode can't parse it (see comment at POST/PUT).
I also, have some initial comments.
Unit tests should be added as well to be able to test the functionality.
...eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/Bookmark.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ecompass/incubator/internal/trace/server/jersey/rest/core/model/BookmarkQueryParameters.java
Show resolved
Hide resolved
...eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/Bookmark.java
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/Bookmark.java
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
2654ee7
to
c2b26d1
Compare
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.
Thanks for the contribution. Code looks good and I appreciate all the details and unit tests. There are few comments that I added.
One more thing, is that the bookmarks implementation via Trace Server is totally separate from the bookmark implementation that is available in classic Trace Compass and hence there is no common ground for reuse. I'm still debating (with myself) if we should align the implementation or if we continue with a separate implementation. I'm aware that the Trace Server implementation and how it's integrate in the clients differs in certain areas from the classic Trace Compass already and it's getting more and more (project model, configuration). One reason for it that certain features in classic Trace Compass have not decoupled UI and core implementation. One big one is the project model (workspace model), which it hard to re-create certain features in the trace server and its clients without re-implementing or painful restructuring in Trace Compass classic. I'll ask other committers to weigh in here so that we find a way forward.
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...ipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/stubs/BookmarkModelStub.java
Outdated
Show resolved
Hide resolved
...rg/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/model/Bookmark.java
Outdated
Show resolved
Hide resolved
...ecompass/incubator/internal/trace/server/jersey/rest/core/model/BookmarkQueryParameters.java
Outdated
Show resolved
Hide resolved
...eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/Bookmark.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Show resolved
Hide resolved
2b4f67e
to
6159498
Compare
...rg/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/model/Bookmark.java
Outdated
Show resolved
Hide resolved
...eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/Bookmark.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
35c3434
to
24be672
Compare
5d50c89
to
f548645
Compare
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
19631bb
to
5d0d640
Compare
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.
Just some formal comments and test cases. Add a test case that validates the error if a experiment doesn't exist on the server (i.e. UUID not available). For that you can have on test method that calls each endpoint with a UUID that doesn't exists on the server.
Otherwise it' looks good to me.
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java
Outdated
Show resolved
Hide resolved
...mpass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java
Outdated
Show resolved
Hide resolved
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.
Looks good to me. Thank your this contribution.
Dismissing the review since Patrick is not available atm. I checked that requested changes are implemented.
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.
The Bookmark tag needs to be added to the TraceServerOpenApiResource, like the others. Otherwise the generated yaml file doesn't have a description for Bookmarks.
7cff0ba
to
8072170
Compare
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.
Looks good to me. Thanks!
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.
do you want to squash or reflow these commits? I am not thrilled with the corrections in the pr being not squashed.
Using bookmarks, user can save custom data points where each corresponds to a specific time range. Each bookmark contains "name", "start", "end", and an optional "payload". The following endpoints are introduced: - [GET] Get all the bookmarks of an experiment - [GET] Get a specific bookmark of an experiment - [POST] Create a new bookmark for an experiment - [PUT] Update an old bookmark of an experiment - [DELETE] Delete a bookmark of an experiment [Added] Corresponding endpoints for bookmarks in trace server Signed-off-by: Kaveh Shahedi <kaveh.shahedi@ericsson.com>
Required unit tests have been implemented for BookmarkServiceManager class. In the tests, various aspects of bookmarking functionality (e.g., creating, updating, deleting, etc.) are checked. [Added] Required unit tests for bookmarking functionality Signed-off-by: Kaveh Shahedi <kaveh.shahedi@ericsson.com>
The new changes aim to re-structure the algorithm of storing bookmarks. Previously, we stored each bookmark (of an experiment) under a specific directory inside the .webapp directory of Trace Compass. Right now, we are using Eclipse's markers system to store and fetch the benchmarks. [Changed] Bookmark storing/fetching system is changed to IMarker Signed-off-by: Kaveh Shahedi <kaveh.shahedi@ericsson.com>
The version is now updated to 0.3.0 from 0.2.0. Most of the changes are related to managing experiments' bookmarks. Also, the bookmarks tag (BKM) is added to TraceServerOpenApiResource class. [Changed] Trace server version is changed to 0.3.0 Signed-off-by: Kaveh Shahedi <kaveh.shahedi@ericsson.com>
8072170
to
1c09447
Compare
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.
LGTM. Thanks!
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.
LGTM
00ad65d
into
eclipse-tracecompass-incubator:master
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.
Looks great! (orphaned commend. Github skill issue on my side.)
Using bookmarks, user can save custom data points where each corresponds to a specific time range. Each bookmark contains "name", "start", "end", and an optional "payload".
The following endpoints are introduced:
[Added] Corresponding endpoints for bookmarks in the trace server