joerghoh commented on code in PR #22:
URL:
https://github.com/apache/sling-org-apache-sling-servlets-post/pull/22#discussion_r1270604051
##
src/main/java/org/apache/sling/servlets/post/impl/operations/ModifyOperation.java:
##
@@ -70,31 +76,54 @@ public void setDateParser(final DateParser dateParser) {
@Override
protected void doRun(final SlingHttpServletRequest request,
-final PostResponse response,
-final List changes)
-throws PersistenceException {
+final PostResponse response,
+final List changes)
+throws PersistenceException {
final Map reqProperties =
collectContent(request, response);
final VersioningConfiguration versioningConfiguration =
getVersioningConfiguration(request);
// do not change order unless you have a very good reason.
+Instant start = Instant.now();
// ensure root of new content
processCreate(request.getResourceResolver(), reqProperties, response,
changes, versioningConfiguration);
+logger.debug("Performed create at {} path took {} ms to operate",
request.getRequestURI(),
Review Comment:
I don't think that it is necessary to consistently add the value of
``request.getRequestURI()`` to the message. This information is always part of
the context of the RequestProgressTracker, and also it won't change while
execution it, it's static.
On top of that, ``Performed create at`` could be understood that a create
is executed at this path, and this is not consistently true, as other resources
could be affected, too (for example sub-resources).
I would just remove this part of the log.
##
src/main/java/org/apache/sling/servlets/post/AbstractPostOperation.java:
##
@@ -167,13 +176,15 @@ public void run(final SlingHttpServletRequest request,
if (isSessionSaveRequired(session, request)) {
request.getResourceResolver().commit();
+log.debug("Saved the session");
}
if (!isSkipCheckin(request)) {
// now do the checkins
for(String checkinPath : nodesToCheckin) {
if (checkin(request.getResourceResolver(), checkinPath)) {
response.onChange("checkin", checkinPath);
+log.debug("Checked in the node");
Review Comment:
same as above
##
src/main/java/org/apache/sling/servlets/post/AbstractPostOperation.java:
##
@@ -167,13 +176,15 @@ public void run(final SlingHttpServletRequest request,
if (isSessionSaveRequired(session, request)) {
request.getResourceResolver().commit();
+log.debug("Saved the session");
Review Comment:
same as above
##
src/main/java/org/apache/sling/servlets/post/AbstractPostOperation.java:
##
@@ -127,6 +135,7 @@ public void run(final SlingHttpServletRequest request,
}
// fail if any of the base paths (before the postfix) which had a
postfix are contained in the modification set
+log.debug("Fail if any of the base paths (before the postfix)
which had a postfix are contained in the modification set");
Review Comment:
What is the purpose of this statement? Is it just for the purpose of having
a timestamp (assuming the log is properly configured to write mili/micro-second
level timestamps)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
