Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
lokeshj1703 commented on code in PR #11149:
URL: https://github.com/apache/hudi/pull/11149#discussion_r1730740068
##
hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/HoodieMultiTableStreamer.java:
##
@@ -430,6 +432,99 @@ public static class Config implements Serializable {
@Parameter(names = {"--help", "-h"}, help = true)
public Boolean help = false;
+
+@Override
+public boolean equals(Object o) {
Review Comment:
Should we consider API like
https://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/commons/lang/builder/EqualsBuilder.html#reflectionEquals(java.lang.Object,%20java.lang.Object)?
The equals and hashCode are not implemented in other config classes as well.
But if we need it we can retain the changes.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
lokeshj1703 commented on code in PR #11149:
URL: https://github.com/apache/hudi/pull/11149#discussion_r1730720592
##
hudi-utilities/src/test/java/org/apache/hudi/utilities/streamer/TestStreamSyncUnitTests.java:
##
@@ -160,6 +165,45 @@ void testGetCheckpointToResume(HoodieStreamer.Config cfg,
HoodieCommitMetadata c
assertEquals(expectedResumeCheckpoint,resumeCheckpoint);
}
+ @ParameterizedTest
+ @MethodSource("getMultiTableStreamerCases")
+ void testCloneConfigsFromMultiTableStreamer(HoodieMultiTableStreamer.Config
cfg) throws IOException {
+Configuration configuration = new Configuration();
+JavaSparkContext jssc = mock(JavaSparkContext.class);
+
+when(jssc.hadoopConfiguration()).thenReturn(configuration);
+
+HoodieMultiTableStreamer multiTableStreamer = new
HoodieMultiTableStreamer(cfg, jssc);
+List tableExecutionContextList =
multiTableStreamer.getTableExecutionContexts();
+tableExecutionContextList.forEach(it -> {
+ // make sure that if set global properties then each child streamer can
get also
+ assertTrue(it.getConfig().configs.containsAll(cfg.configs));
Review Comment:
I just wanted to ensure we have a test which covers the case where the
configs are non empty. I see that is already covered based on the test cases.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
hwani3142 commented on code in PR #11149:
URL: https://github.com/apache/hudi/pull/11149#discussion_r1728904841
##
hudi-utilities/src/test/java/org/apache/hudi/utilities/streamer/TestStreamSyncUnitTests.java:
##
@@ -160,6 +165,45 @@ void testGetCheckpointToResume(HoodieStreamer.Config cfg,
HoodieCommitMetadata c
assertEquals(expectedResumeCheckpoint,resumeCheckpoint);
}
+ @ParameterizedTest
+ @MethodSource("getMultiTableStreamerCases")
+ void testCloneConfigsFromMultiTableStreamer(HoodieMultiTableStreamer.Config
cfg) throws IOException {
+Configuration configuration = new Configuration();
+JavaSparkContext jssc = mock(JavaSparkContext.class);
+
+when(jssc.hadoopConfiguration()).thenReturn(configuration);
+
+HoodieMultiTableStreamer multiTableStreamer = new
HoodieMultiTableStreamer(cfg, jssc);
+List tableExecutionContextList =
multiTableStreamer.getTableExecutionContexts();
+tableExecutionContextList.forEach(it -> {
+ // make sure that if set global properties then each child streamer can
get also
+ assertTrue(it.getConfig().configs.containsAll(cfg.configs));
Review Comment:
@lokeshj1703
We should allow to have empty configs.
[Test case
1](https://github.com/apache/hudi/pull/11149/files#diff-d5d9e34abfadbfc00ca3aa1642dbfde1a59ecaf2976ce7a5be418e9ae0a1a17fR192)
address that.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
hwani3142 commented on code in PR #11149:
URL: https://github.com/apache/hudi/pull/11149#discussion_r1728901184
##
hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/HoodieMultiTableStreamer.java:
##
@@ -430,6 +432,99 @@ public static class Config implements Serializable {
@Parameter(names = {"--help", "-h"}, help = true)
public Boolean help = false;
+
+@Override
+public boolean equals(Object o) {
Review Comment:
I think that every config class should have equals, hashCode, toString for
convenience.
If not, can rollback them.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
lokeshj1703 commented on code in PR #11149:
URL: https://github.com/apache/hudi/pull/11149#discussion_r1728734925
##
hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/HoodieMultiTableStreamer.java:
##
@@ -430,6 +432,99 @@ public static class Config implements Serializable {
@Parameter(names = {"--help", "-h"}, help = true)
public Boolean help = false;
+
+@Override
+public boolean equals(Object o) {
Review Comment:
Do we need the equals and toString implementation here?
##
hudi-utilities/src/test/java/org/apache/hudi/utilities/streamer/TestStreamSyncUnitTests.java:
##
@@ -160,6 +165,45 @@ void testGetCheckpointToResume(HoodieStreamer.Config cfg,
HoodieCommitMetadata c
assertEquals(expectedResumeCheckpoint,resumeCheckpoint);
}
+ @ParameterizedTest
+ @MethodSource("getMultiTableStreamerCases")
+ void testCloneConfigsFromMultiTableStreamer(HoodieMultiTableStreamer.Config
cfg) throws IOException {
+Configuration configuration = new Configuration();
+JavaSparkContext jssc = mock(JavaSparkContext.class);
+
+when(jssc.hadoopConfiguration()).thenReturn(configuration);
+
+HoodieMultiTableStreamer multiTableStreamer = new
HoodieMultiTableStreamer(cfg, jssc);
+List tableExecutionContextList =
multiTableStreamer.getTableExecutionContexts();
+tableExecutionContextList.forEach(it -> {
+ // make sure that if set global properties then each child streamer can
get also
+ assertTrue(it.getConfig().configs.containsAll(cfg.configs));
Review Comment:
Can we also add a check for `cfg.configs` to be non empty?
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
fijemax commented on PR #11149: URL: https://github.com/apache/hudi/pull/11149#issuecomment-2289147386 Any news on this issue ? -- 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: commits-unsubscr...@hudi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
hudi-bot commented on PR #11149: URL: https://github.com/apache/hudi/pull/11149#issuecomment-2094335208 ## CI report: * bb03750d5e951785c6205f501d463614cc3315cf Azure: [SUCCESS](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23657) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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: commits-unsubscr...@hudi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
hudi-bot commented on PR #11149: URL: https://github.com/apache/hudi/pull/11149#issuecomment-2094305741 ## CI report: * bb03750d5e951785c6205f501d463614cc3315cf Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23657) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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: commits-unsubscr...@hudi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
hudi-bot commented on PR #11149: URL: https://github.com/apache/hudi/pull/11149#issuecomment-2094288851 ## CI report: * bb03750d5e951785c6205f501d463614cc3315cf UNKNOWN Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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: commits-unsubscr...@hudi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [HUDI-7711] Fix MultiTableStreamer can deal with path of properties files [hudi]
hwani3142 opened a new pull request, #11149: URL: https://github.com/apache/hudi/pull/11149 ### Change Logs fix copy logic on MultiTableStreamer ### Impact HoodieMultiTableStreamer ### Risk level (write none, low medium or high below) low ### Documentation Update none ### Contributor's checklist - [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute) - [ ] Change Logs and Impact were stated clearly - [ ] Adequate tests were added if applicable - [ ] CI passed -- 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: commits-unsubscr...@hudi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
