server: Add bookmarks endpoint support #102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bhufmann
requested changes
Oct 31, 2024
There was a problem hiding this comment.
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
kavehshahedi
force-pushed
the
bookmarks-endpoint
branch
2 times, most recently
from
2654ee7 to
c2b26d1
Compare
bhufmann
requested changes
Nov 11, 2024
There was a problem hiding this comment.
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
kavehshahedi
force-pushed
the
bookmarks-endpoint
branch
from
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
PatrickTasse
requested changes
Nov 13, 2024
...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
kavehshahedi
force-pushed
the
bookmarks-endpoint
branch
3 times, most recently
from
35c3434 to
24be672
Compare
kavehshahedi
force-pushed
the
bookmarks-endpoint
branch
5 times, most recently
from
5d50c89 to
f548645
Compare
PatrickTasse
previously requested changes
Nov 18, 2024
...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
kavehshahedi
force-pushed
the
bookmarks-endpoint
branch
3 times, most recently
from
19631bb to
5d0d640
Compare
bhufmann
requested changes
Nov 21, 2024
There was a problem hiding this comment.
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
bhufmann
previously approved these changes
Nov 22, 2024
bhufmann
dismissed
PatrickTasse’s stale review
Dismissing the review since Patrick is not available atm. I checked that requested changes are implemented.
bhufmann
approved these changes
Nov 22, 2024
bhufmann
requested changes
Nov 22, 2024
kavehshahedi
force-pushed
the
bookmarks-endpoint
branch
from
7cff0ba to
8072170
Compare
bhufmann
approved these changes
Nov 22, 2024
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
kavehshahedi
force-pushed
the
bookmarks-endpoint
branch
from
8072170 to
1c09447
Compare
bhufmann
merged commit 00ad65d
into
eclipse-tracecompass-incubator:master
2 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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