nsivabalan commented on a change in pull request #4777:
URL: https://github.com/apache/hudi/pull/4777#discussion_r806023345
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -1142,7 +1152,13 @@ protected boolean scheduleCleaningAtInstant(String
instantTime, Option<Map<Strin
private Option<String> scheduleTableServiceInternal(String instantTime,
Option<Map<String, String>> extraMetadata,
TableServiceType
tableServiceType) {
+ if (!tableServicesEnabled(config)) {
+ return Option.empty();
+ }
switch (tableServiceType) {
+ case ARCHIVE:
+ LOG.info("Scheduling archiving is not supported. Skipping.");
Review comment:
why adding this? no one should be scheduling archiving. so, wondering if
we should not add this and let it throw in default clause w/ unknown table
service type ?
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/async/HoodieAsyncTableService.java
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.async;
+
+import org.apache.hudi.client.RunsTableService;
+import org.apache.hudi.config.HoodieWriteConfig;
+
+import java.util.function.Function;
+
+public abstract class HoodieAsyncTableService extends HoodieAsyncService
implements RunsTableService {
Review comment:
I see a reason to have RunsTableService, but whats the necessity to add
HoodieAsyncService and then HoodieAsyncTableService. I feel we can drop
HoodieAsyncService only. and HoodieAsyncTableService can directly implement
RunsTableService.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/async/HoodieAsyncTableService.java
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.async;
+
+import org.apache.hudi.client.RunsTableService;
+import org.apache.hudi.config.HoodieWriteConfig;
+
+import java.util.function.Function;
+
+public abstract class HoodieAsyncTableService extends HoodieAsyncService
implements RunsTableService {
+
+ protected HoodieWriteConfig writeConfig;
+
+ protected HoodieAsyncTableService() {
+ }
+
+ protected HoodieAsyncTableService(HoodieWriteConfig writeConfig) {
+ this.writeConfig = writeConfig;
+ }
+
+ protected HoodieAsyncTableService(HoodieWriteConfig writeConfig, boolean
runInDaemonMode) {
+ super(runInDaemonMode);
+ this.writeConfig = writeConfig;
+ }
+
+ @Override
+ public void start(Function<Boolean, Boolean> onShutdownCallback) {
+ if (!tableServicesEnabled(writeConfig)) {
Review comment:
also, trying to understand where exactly this guard need to be added.
As per master, this is my understanding.
we check the configs and may not start the async service only.
for eg
```
if (writeClient.getConfig().isAutoClean() &&
writeClient.getConfig().isAsyncClean()) {
asyncCleanerService = new AsyncCleanerService(writeClient);
asyncCleanerService.start(null);
} else {
LOG.info("Async auto cleaning is not enabled. Not running cleaner
now");
}
```
So, that the shutdown code looks as below.
```
if (asyncCleanerService != null) {
LOG.info("Waiting for async cleaner to finish");
try {
asyncCleanerService.waitForShutdown();
} catch (Exception e) {
throw new HoodieException("Error waiting for async cleaning to
finish", e);
}
}
```
```
public static void forceShutdown(AsyncCleanerService asyncCleanerService) {
if (asyncCleanerService != null) {
LOG.info("Shutting down async cleaner");
asyncCleanerService.shutdown(true);
}
}
```
but now as per this patch, I feel this is an additional check we are adding
which may not be invoked at all since the async service should not be
instantiated at all if the config of interest is not enabled only.
Or how are we ensuring HoodieAsyncService.shutdown() and
HoodieAsyncService.waitForShutdown accounts for tableServiceDisabled flow.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/async/HoodieAsyncTableService.java
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.async;
+
+import org.apache.hudi.client.RunsTableService;
+import org.apache.hudi.config.HoodieWriteConfig;
+
+import java.util.function.Function;
+
+public abstract class HoodieAsyncTableService extends HoodieAsyncService
implements RunsTableService {
Review comment:
I see a reason to have RunsTableService, but whats the necessity to add
HoodieAsyncService and then HoodieAsyncTableService. I feel we can drop
HoodieAsyncService only. and HoodieAsyncTableService can directly implement
RunsTableService. the functionality you intend to add to HoodieAsyncService, we
can add it to HoodieAsyncTableService only. or if you can help explain the
need, would be great.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/RunsTableService.java
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.client;
+
+import org.apache.hudi.config.HoodieWriteConfig;
+
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+public interface RunsTableService {
+
+ Logger LOG = LogManager.getLogger(RunsTableService.class);
+
+ default boolean tableServicesEnabled(HoodieWriteConfig config) {
+ boolean enabled = config.areTableServicesEnabled();
Review comment:
on second thought, wondering if we really need this class. this is just
one line condition check and its just one config check. why not check it in
place only.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/async/HoodieAsyncTableService.java
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.async;
+
+import org.apache.hudi.client.RunsTableService;
+import org.apache.hudi.config.HoodieWriteConfig;
+
+import java.util.function.Function;
+
+public abstract class HoodieAsyncTableService extends HoodieAsyncService
implements RunsTableService {
+
+ protected HoodieWriteConfig writeConfig;
+
+ protected HoodieAsyncTableService() {
+ }
+
+ protected HoodieAsyncTableService(HoodieWriteConfig writeConfig) {
+ this.writeConfig = writeConfig;
+ }
+
+ protected HoodieAsyncTableService(HoodieWriteConfig writeConfig, boolean
runInDaemonMode) {
+ super(runInDaemonMode);
+ this.writeConfig = writeConfig;
+ }
+
+ @Override
+ public void start(Function<Boolean, Boolean> onShutdownCallback) {
+ if (!tableServicesEnabled(writeConfig)) {
Review comment:
sounds good.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
