Lazy init of fullyQualifiedStorageDirectory in HDFS pusher #5684
Conversation
|
@jon-wei LGTM but i don't this this is a bug, i bet it is a config issue. |
| @@ -230,4 +231,19 @@ public String makeIndexPathName(DataSegment dataSegment, String indexName) | |||
| indexName | |||
| ); | |||
| } | |||
|
|
|||
| private void initFullyQualifiedStorageDirectory() | |||
There was a problem hiding this comment.
Would you please add a comment about why this should be lazily initialized?
There was a problem hiding this comment.
Whoops, forgot to push that change, added a comment
|
@b-slim Thanks for the review, I removed the bug label for now |
| @@ -53,7 +53,8 @@ | |||
|
|
|||
| private final Configuration hadoopConfig; | |||
| private final ObjectMapper jsonMapper; | |||
| private final String fullyQualifiedStorageDirectory; | |||
| private final Path storageDir; | |||
| private String fullyQualifiedStorageDirectory; | |||
There was a problem hiding this comment.
This should be volatile since the object is shared amongst multiple threads.
There was a problem hiding this comment.
Went with the memoize approach
| private void initFullyQualifiedStorageDirectory() | ||
| { | ||
| try { | ||
| if (fullyQualifiedStorageDirectory == null) { |
There was a problem hiding this comment.
If it was a conscious decision to not synchronize this, say why in a comment. Or if it wasn't a conscious decision, consider synchronizing it.
There was a problem hiding this comment.
Maybe https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Suppliers.html#memoize(com.google.common.base.Supplier)? Might be cleaner than calling init in places too
There was a problem hiding this comment.
Went with the memoize approach
| @@ -230,4 +231,22 @@ public String makeIndexPathName(DataSegment dataSegment, String indexName) | |||
| indexName | |||
| ); | |||
| } | |||
|
|
|||
|
|
|||
| // We lazily initialiize fullQualifiedStorageDirectory to avoid potential issues with Hadoop namenode HA. | |||
* Lazy init of fullyQualifiedStorageDirectory in HDFS pusher * Comment * Fix test * PR comments
Some users have encountered issues when running Druid 0.12.0 against a Hadoop cluster with namenode HA enabled, where the mapper encounters an UnknownHostException when it tries to interpret the logical nameservice string as a hostname (e.g.: #5552). The users encountering issue also report that their hadoop ingestion was working in Druid 0.10.0 with the same hadoop configs.
In a local test environment, I was able to run a hadoop ingestion task successfully using namenode HA (#5552 (comment)), so this issue might be a misconfiguration in some cases.
However, in another case, we observed this issue in a customer environment where the Hadoop .xml files in the Druid classpath were correctly configured for namenode HA, but the mapper failed to pick up the configurations. The root cause was unclear.
Given that the mapper does not actually need the DataSegmentPusher, this PR is a workaround that lazily evaluates the the
fullyQualifiedStorageDirectoryvariable in HdfsDataSegmentPusher, to unblock users who are encountering this issue, until a more complete understanding of the issue is reached.I suspect that these users did not encounter issues in 0.10.0 because in that version HadoopDruidIndexerConfig did not have the DataSegmentPusher as a field (see #4116).