-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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.
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
HDDS-1696. RocksDB use separate Write-ahead-log location for OM RocksDB. #981
Conversation
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned that this is a feature that might be just over-engineering. We have always claimed that OM or SCM needs SSDs. If we have SSDs I don't think we will be bottlenecked by the IOPS. I know that RocksDB has lots of features, not all we need to expose in ozone. Also, RocksDB settings are isolated today in a class called RockDB config. It really think we should not bleed them into Ozone.
@@ -35,6 +35,7 @@ | |||
// metadata dir but in future we may support multiple for redundancy or | |||
// performance. | |||
public static final String OZONE_SCM_DB_DIRS = "ozone.scm.db.dirs"; | |||
public static final String OZONE_SCM_DB_WAL_DIR = "ozone.scm.db.wal.dir"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know that WAL performance is being compromised today ? This adds a new config value, but are we sure about the benefit ? In other words, are we sure that ozone is being bottlenecked by RocksDB?
@@ -220,4 +227,15 @@ private File getDBFile() throws IOException { | |||
return Paths.get(dbPath.toString(), dbname).toFile(); | |||
} | |||
|
|||
private File getWALFile() throws IOException { | |||
if (walPath == null) { | |||
LOG.error("Write-ahead-log path is " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in most cases this would not happen. No one would map a WAL to a different disk. If it is mapped, then we can probably log that we detected it is mapped. This is certainly not an error.
@@ -646,6 +646,17 @@ | |||
production environments. | |||
</description> | |||
</property> | |||
<property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please use the new config style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -699,6 +710,17 @@ | |||
production environments. | |||
</description> | |||
</property> | |||
<property> | |||
<name>ozone.scm.db.wal.dir</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above.
@@ -2327,6 +2349,17 @@ | |||
production environments. | |||
</description> | |||
</property> | |||
<property> | |||
<name>ozone.recon.db.wal.dir</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same.
@@ -35,6 +35,7 @@ | |||
// metadata dir but in future we may support multiple for redundancy or | |||
// performance. | |||
public static final String OZONE_SCM_DB_DIRS = "ozone.scm.db.dirs"; | |||
public static final String OZONE_SCM_DB_WAL_DIR = "ozone.scm.db.wal.dir"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have data that indicates the compaction is the root cause of issues? On a normal SSD, we have around 80K IOPS are we saying that we are need more bandwidth than that ?
@@ -220,4 +227,15 @@ private File getDBFile() throws IOException { | |||
return Paths.get(dbPath.toString(), dbname).toFile(); | |||
} | |||
|
|||
private File getWALFile() throws IOException { | |||
if (walPath == null) { | |||
LOG.error("Write-ahead-log path is " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that most people will never even add WAL as an option, so this should not be an error IMHO.
@@ -646,6 +646,17 @@ | |||
production environments. | |||
</description> | |||
</property> | |||
<property> | |||
<name>ozone.om.db.wal.dir</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use new style Config options please.
Assert.assertTrue(newFolder.mkdirs()); | ||
} | ||
DBStoreBuilder.newBuilder(conf) | ||
.setPath(newFolder.toPath()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test illustrates my concern. We will add this feature for performance reasons but we really have no data to show or prove this is the root cause of perf issues. Neither can we test it.
// If wal Dir is set, as in OM HA setup all OM's will be on the | ||
// same node, wal directories will conflict with each other. | ||
// Append nodeId similar to metaDirPath. | ||
String walDir = conf.get(OMConfigKeys.OZONE_OM_DB_WAL_DIR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New Configs please.
Closing this, as there is a way to pass a .ini file and read the values and set the required rocksdb. |
…s thread-safe, enabling StreamRegexMonitors only when required StreamPartitionCountMOnitor and StreamRegexMonitor use the KafkaSystemAdmin, which has a kafka-consumer. Kafka consumer is not thread-safe, therefore explicit synchronization is required for metadata accesses. StreamRegexMonitor is not to be enabled when there is no regex input. Author: Ray Matharu <rmatharu@linkedin.com> Reviewers: Prateek Maheshwari <pmaheshwari@apache.org> Closes apache#981 from rmatharu/bugfix-regexmonitor
No description provided.