You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ArbaazKhan1 (via GitHub)" <gi...@apache.org> on 2024/01/24 22:15:12 UTC
[PR] Refactored DfsLogger constructors to static methods that now call a p… [accumulo]
ArbaazKhan1 opened a new pull request, #4192:
URL: https://github.com/apache/accumulo/pull/4192
I made the dfslogger constructors private and now have them being called from static methods. To ensure that LogEntry can now be set as final in dfslogger, I removed the instantiation of it from the open() method and now have it initialized it in the newly created static method named fromCounters().
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactored DfsLogger constructors to static methods that now call a p… [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #4192:
URL: https://github.com/apache/accumulo/pull/4192#discussion_r1482108135
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java:
##########
@@ -291,16 +291,40 @@ public int hashCode() {
private final ServerContext context;
private FSDataOutputStream logFile;
private DataOutputStream encryptingLogFile = null;
- private LogEntry logEntry;
+ private final LogEntry logEntry;
private Thread syncThread;
private AtomicLong syncCounter;
private AtomicLong flushCounter;
private final long slowFlushMillis;
private long writes = 0;
- public DfsLogger(ServerContext context, AtomicLong syncCounter, AtomicLong flushCounter) {
- this(context, null);
+ public static DfsLogger fromCounters(ServerContext context, AtomicLong syncCounter,
Review Comment:
I pushed a different name in my code review commit, in collab with @ArbaazKhan1
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactored DfsLogger constructors to static methods that now call a p… [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4192:
URL: https://github.com/apache/accumulo/pull/4192#issuecomment-1933020530
Thanks for the PR @ArbaazKhan1. I see you aren't listed on our "people" page, acknowledging contributors. If you wish to be added as a contributor to https://accumulo.apache.org/people/ , please open a pull request to add yourself at https://github.com/apache/accumulo-website/edit/main/pages/people.md and leave a reference to `apache/accumulo#4192` in your commit log.
If you intend to be a regular contributor to Accumulo projects, please consider subscribing to our developer mailing list (https://accumulo.apache.org/contact-us/) and introducing yourself. :smiley_cat:
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactored DfsLogger constructors to static methods that now call a p… [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4192:
URL: https://github.com/apache/accumulo/pull/4192#discussion_r1466687596
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java:
##########
@@ -291,16 +291,36 @@ public int hashCode() {
private final ServerContext context;
private FSDataOutputStream logFile;
private DataOutputStream encryptingLogFile = null;
- private LogEntry logEntry;
+ private final LogEntry logEntry;
private Thread syncThread;
private AtomicLong syncCounter;
private AtomicLong flushCounter;
private final long slowFlushMillis;
private long writes = 0;
- public DfsLogger(ServerContext context, AtomicLong syncCounter, AtomicLong flushCounter) {
- this(context, null);
+ public static DfsLogger fromCounters(ServerContext context, AtomicLong syncCounter,
+ AtomicLong flushCounter, String address) {
+
+ String filename = UUID.randomUUID().toString();
+ String logger = Joiner.on("+").join(address.split(":"));
+ VolumeManager fs = context.getVolumeManager();
+
+ var chooserEnv =
+ new VolumeChooserEnvironmentImpl(VolumeChooserEnvironment.Scope.LOGGER, context);
+ String logPath = fs.choose(chooserEnv, context.getBaseUris()) + Path.SEPARATOR
+ + Constants.WAL_DIR + Path.SEPARATOR + logger + Path.SEPARATOR + filename;
+
+ return new DfsLogger(context, syncCounter, flushCounter, LogEntry.fromPath(logPath));
+ }
+
+ public static DfsLogger fromExistingLogEntry(ServerContext context, LogEntry logEntry) {
+ return new DfsLogger(context, logEntry);
+ }
+
+ private DfsLogger(ServerContext context, AtomicLong syncCounter, AtomicLong flushCounter,
+ LogEntry logEntry) {
Review Comment:
This is pretty similar to another approach that I thought of that makes it so we can call open within the `fromCounters` method.
What we might be able to do is make a single private constructor that sets all of the fields that both of these static methods use. The `fromExistingLogEntry` is more straight forward where we can just call the private constructor but the `fromCounters` we could do the following:
After creating the `String logPath`, create the `LogEntry` and use all of the variables to create a DfsLogger object to then call `open` on. Then return that DfsLogger. Like this:
```java
String logPath = fs.choose(chooserEnv, context.getBaseUris()) + Path.SEPARATOR
+ Constants.WAL_DIR + Path.SEPARATOR + loggerAddress + Path.SEPARATOR + filename;
LogEntry logEntry = LogEntry.fromPath(logPath);
DfsLogger logger = new DfsLogger(context, logEntry, syncCounter, flushCounter);
logger.open(fs, logPath, filename);
return logger;
```
That way we can make open private too and LogEntry can be final.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java:
##########
@@ -266,7 +266,8 @@ private synchronized void startLogMaker() {
DfsLogger alog = null;
try {
- alog = new DfsLogger(tserver.getContext(), syncCounter, flushCounter);
+ alog = DfsLogger.fromCounters(tserver.getContext(), syncCounter, flushCounter,
+ tserver.getClientAddressString());
alog.open(tserver.getClientAddressString());
Review Comment:
This is the spot referenced in the ticket and I think can be addressed by my other comments.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactored DfsLogger constructors to static methods that now call a p… [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #4192:
URL: https://github.com/apache/accumulo/pull/4192
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactored DfsLogger constructors to static methods that now call a p… [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4192:
URL: https://github.com/apache/accumulo/pull/4192#discussion_r1470229000
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java:
##########
@@ -291,16 +291,40 @@ public int hashCode() {
private final ServerContext context;
private FSDataOutputStream logFile;
private DataOutputStream encryptingLogFile = null;
- private LogEntry logEntry;
+ private final LogEntry logEntry;
private Thread syncThread;
private AtomicLong syncCounter;
private AtomicLong flushCounter;
private final long slowFlushMillis;
private long writes = 0;
- public DfsLogger(ServerContext context, AtomicLong syncCounter, AtomicLong flushCounter) {
- this(context, null);
+ public static DfsLogger fromCounters(ServerContext context, AtomicLong syncCounter,
Review Comment:
I'm not sure of the best name for this method but maybe something like `fromNewLogEntry` might be a good fit too. I am also fine with leaving it as is so unless someone else has an opinion I think its fine.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org