Provide a way to get last file revisions #1096

ikhoon · 2025-02-10T15:52:40Z

Motivation:

Central Dogma's content API does not return a file revision when the file is modified.
Alternatively, users can use the history API to find the file revision. One downside of the history API is that we couldn't use it to find the last file revisions of multiple files at once, requiring N queries instead.

If the content API can directly return the last file revision, it would eliminate the need to use the history API and execute N queries. Additionally, since the request is simple, caching would be beneficial from the server’s perspective. If multiple servers in the cluster perform the same operation due to a specific event, the cache hit rate is expected to be high.

Modifications:

REST API)
- Add includeLastFileRevision (int) as a query parameter to listFiles and getFiles API. If the value is greater than 1, EntryDto may include the modified file revision.
Java Client API)
- FilesWithRevisionRequest is added to fluent specify includeLastFileRevision in the get files or list files API.
Internal)
- Add blockingFindLastFileRevisions() to search for file revision within a range of revisions. If a file is not found in the revisions, Revision.INIT (1) is filled instead.

Result:

You can now retrieve the last file revisions using the list files API and the get files API.

CentralDogmaRepository repo = client.forRepo("myProj", "myRepo");

Map<String, Entry<?>> listFiles = 
  repo.file(PathPattern.all())
      // Specify the maximum number of revisions to search for the last file revision.
      // If the last file revision is not found from the specified range of revisions, 
      // `Revision.INIT` is returned instead.
      .includeLastFileRevision(100)
      .list()
      .join();

jrhee17 · 2025-02-12T01:10:43Z

...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java

@@ -408,29 +408,68 @@ private static Revision normalizeRevision(AggregatedHttpResponse res) {
    @Override
    public CompletableFuture<Map<String, EntryType>> listFiles(String projectName, String repositoryName,
                                                               Revision revision, PathPattern pathPattern) {
+        return listFiles0(projectName, repositoryName, revision, pathPattern, -1,


Question) What do you think of setting the default value as 1 for both client/server side since it seems like 1 represents "don't search the history"?

Personally, I think it makes more sense that:

0 represents "check backwards history for 0 revisions"

validate on includeLastFileRevision<0

The current implementation seems fine though.

0 represents "check backwards history for 0 revisions"

If so, should 0 and 1 return the same result?

Personally, I think it makes more sense that:
0 represents "check backwards history for 0 revisions"
validate on includeLastFileRevision<0

I meant it feels more intuitive to me that the parameter represents the number of revisions to go backwards. (i.e. Revision#backward(int)).

I think the current implementation is fine as long as 1) the default value is well defined with validation 2) and the javadocs are clear. It might be helpful if the parameter name better represents the meaning of "check x number of revision including the head revision" as well.

jrhee17 · 2025-02-12T02:22:34Z

...a/com/linecorp/centraldogma/server/internal/storage/repository/cache/CacheableQueryCall.java

        super(repo);
        this.revision = requireNonNull(revision, "revision");
        this.query = requireNonNull(query, "query");
+        this.includeLastFileRevision = includeLastFileRevision <= 1 ? -1 : includeLastFileRevision;


Do we not need to check equality based on includeLastFileRevision as well?

That's my mistake.

minwoox

Looks great. 👍 Left some suggestions. 😉

minwoox · 2025-02-13T03:51:30Z

...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java


                return client.execute(headers(HttpMethod.GET, path.toString()))
                             .aggregate()
-                             .thenApply(res -> getFiles(normRev, res));
+                             .thenApply(res -> getFiles(res));


Suggested change

.thenApply(res -> getFiles(res));

.thenApply(ArmeriaCentralDogma::getFiles);

minwoox · 2025-02-13T06:02:45Z

common/src/main/java/com/linecorp/centraldogma/common/Entry.java

+     */
+    public Entry<T> withRevision(Revision revision) {
+        requireNonNull(revision, "revision");
+        return new Entry<>(revision, path, type, content, true);


Probably cache the allowNullContent and reuse it here for better accuracy?

minwoox · 2025-02-13T06:07:01Z

common/src/main/java/com/linecorp/centraldogma/common/Entry.java

+     * @param type the type of the {@link Entry}
+     * @param <T> the content type. {@link JsonNode} if JSON. {@link String} if text.
+     */
+    public static <T> Entry<T> ofNullable(Revision revision, String path, EntryType type, @Nullable T content) {


What do you think of removing the content from the parameters if you want to make a nullable content entry?
Otherwise, I think we can use the above method which takes a @Nullable T content.

minwoox · 2025-02-13T06:23:53Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

+                                                 .filter(entry -> entry.type().type() != Void.class)
+                                                 // Remove the heading `/`
+                                                 .map(entry -> entry.path().substring(1))
+                                                 .collect(Collectors.toList());


minwoox · 2025-02-13T06:25:33Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

+        final Map<String, Revision> lastRevisionMap = new HashMap<>();
+
+        // At this point, we are sure: from.major >= to.major and read lock is acquired.
+        try (ObjectReader reader = jGitRepository.newObjectReader();


What do you think of using the caching object reader that is introduced in #1097? 😉

minwoox · 2025-02-13T06:30:09Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

+                if (revCommit.getParentCount() == 0) {
+                    break; // Reached the root commit
+                }
+


Probably we can add:

assert revCommit.getParentCount() == 1;

minwoox · 2025-02-13T06:36:46Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

+                treeWalk.reset();
+                treeWalk.addTree(revCommit.getTree());
+                treeWalk.addTree(revCommit.getParent(0).getTree());


We can do:

Suggested change

treeWalk.reset();

treeWalk.addTree(revCommit.getTree());

treeWalk.addTree(revCommit.getParent(0).getTree());

treeWalk.reset(revCommit.getTree().getId(), revCommit.getParent(0).getTree());

minwoox · 2025-02-13T06:41:45Z

server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java

+     *
+     *


Suggested change

*

*

*

Provide a way to get last file revisions

4d7ef75

ikhoon added the new feature label Feb 10, 2025

ikhoon added 2 commits February 11, 2025 13:28

Add tests and minor fixes

ef8bdc8

fix tests

d16e17f

ikhoon added this to the 0.74.0 milestone Feb 11, 2025

fix broken tests

3945282

ikhoon marked this pull request as ready for review February 11, 2025 05:10

ikhoon requested review from jrhee17, minwoox and trustin as code owners February 11, 2025 05:10

jrhee17 approved these changes Feb 12, 2025

View reviewed changes

address comments

c2af4b5

minwoox reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to get last file revisions #1096

Provide a way to get last file revisions #1096

ikhoon commented Feb 10, 2025 •

edited

Loading

jrhee17 Feb 12, 2025

ikhoon Feb 12, 2025

ikhoon Feb 12, 2025

jrhee17 Feb 13, 2025

jrhee17 Feb 12, 2025

ikhoon Feb 12, 2025

minwoox left a comment

minwoox Feb 13, 2025

minwoox Feb 13, 2025

minwoox Feb 13, 2025

minwoox Feb 13, 2025

minwoox Feb 13, 2025

minwoox Feb 13, 2025

minwoox Feb 13, 2025

minwoox Feb 13, 2025

	.thenApply(res -> getFiles(res));
	.thenApply(ArmeriaCentralDogma::getFiles);

Provide a way to get last file revisions #1096

Are you sure you want to change the base?

Provide a way to get last file revisions #1096

Conversation

ikhoon commented Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Feb 10, 2025 •

edited

Loading