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.

Sign up for GitHub

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

Jump to bottom

Simple date format each time new object creation removed #3027

Closed
wants to merge 10 commits into from
Closed

Simple date format each time new object creation removed #3027

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7956c15
Simple date format each time new object creation removed
khanimteyaz Dec 20, 2018
fae5fa8
update dependencies in readme (#3028)
beiwei30 Dec 20, 2018
3e6ae66
Fixing typos (#3030)
sergioescala Dec 21, 2018
5607b0a
Simple date format each time new object creation removed
khanimteyaz Dec 20, 2018
5d3398b
incorporated review comment and minor refactoring
khanimteyaz Dec 21, 2018
89e5485
incorporated review comment and minor refactoring
khanimteyaz Dec 21, 2018
1288f12
incorporated review comment
khanimteyaz Dec 26, 2018
98adaac
review comment incorporated, log message creation moved to separate t…
khanimteyaz Dec 26, 2018
59188bc
review comment incorporated, log message creation moved to separate t…
khanimteyaz Dec 26, 2018
f53137d
fixed ASF license issue
khanimteyaz Dec 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/filter/AccessLogFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,20 @@ public class AccessLogFilter implements Filter {

private static final long LOG_OUTPUT_INTERVAL = 5000;

static ThreadLocal<SimpleDateFormat> FILE_DATE_FORMATTER= new ThreadLocal<SimpleDateFormat>() {
@Override
protected SimpleDateFormat initialValue() {
return new SimpleDateFormat(FILE_DATE_FORMAT);
}
};

static ThreadLocal<SimpleDateFormat> MESSAGE_DATE_FORMATTER= new ThreadLocal<SimpleDateFormat>() {
Copy link
Member

Choose a reason for hiding this comment

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

Hi, khan
We have integrated Netty's FastThreadLocal in dubbo. I think we can use it instead of ThreadLocal. I suggest you refer to RpcContext.LOCAL.

Copy link
Contributor Author

@khanimteyaz khanimteyaz Dec 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for suggesting this. I was looking into RpcContext code for LOCAL and SERVER_LOCAL implementation, I would like to go this approach.
I am very curious to know, the difference between LOCAL and SERVER_LOCAL here. Is LOCAL is used for when the call is from consumer and SERVER_LOCAL is used for,when call is served by SERVER? Is this understanding of mine is correct?

@Override
protected SimpleDateFormat initialValue() {
return new SimpleDateFormat(MESSAGE_DATE_FORMAT);
}
};

private final ConcurrentMap<String, Set<String>> logQueue = new ConcurrentHashMap<String, Set<String>>();

private final ScheduledExecutorService logScheduled = Executors.newScheduledThreadPool(2, new NamedThreadFactory("Dubbo-Access-Log", true));
Expand Down Expand Up @@ -113,7 +127,7 @@ public Result invoke(Invoker<?> invoker, Invocation inv) throws RpcException {
String version = invoker.getUrl().getParameter(Constants.VERSION_KEY);
String group = invoker.getUrl().getParameter(Constants.GROUP_KEY);
StringBuilder sn = new StringBuilder();
sn.append("[").append(new SimpleDateFormat(MESSAGE_DATE_FORMAT).format(new Date())).append("] ").append(context.getRemoteHost()).append(":").append(context.getRemotePort())
sn.append("[").append(MESSAGE_DATE_FORMATTER.get().format(new Date())).append("] ").append(context.getRemoteHost()).append(":").append(context.getRemotePort())
Copy link
Member

Choose a reason for hiding this comment

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

Another thing we should consider is that when we clear the threadlocal's value.
Should we keep them in memory all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carryxyh
If the current thread which execute this invoke is from a thread pool then I think we should keep it because then it will be reusable and will avoid the time creating new date formatter object, otherwise we should remove it.
What would you suggest to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carryxyh would you suggest is there any things I needs to change here?

.append(" -> ").append(context.getLocalHost()).append(":").append(context.getLocalPort())
.append(" - ");
if (null != group && group.length() > 0) {
Expand Down Expand Up @@ -174,8 +188,8 @@ public void run() {
logger.debug("Append log to " + accesslog);
}
if (file.exists()) {
String now = new SimpleDateFormat(FILE_DATE_FORMAT).format(new Date());
String last = new SimpleDateFormat(FILE_DATE_FORMAT).format(new Date(file.lastModified()));
String now = FILE_DATE_FORMATTER.get().format(new Date());
String last = FILE_DATE_FORMATTER.get().format(new Date(file.lastModified()));
if (!now.equals(last)) {
File archive = new File(file.getAbsolutePath() + "." + last);
file.renameTo(archive);
Expand Down