Remove unnecessary synchronization

synchronized is not necessary, because the sync operation already has WriteLock to ensure synchronization

## JIRA info

- Link: https://issues.apache.org/jira/browse/HUDI-1982
- Type: Improvement

---

## Comments

04/Jul/21 09:51;zhangway;[~vinoth] Can you  give this a look or commet ?;;;

---

08/Jul/21 06:57;githubbot;vinothchandar commented on a change in pull request #3041:
URL: https://github.com/apache/hudi/pull/3041#discussion_r665919857

##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java
##########
@@ -132,15 +132,13 @@ private boolean syncIfLocalViewBehind(Context ctx) {
String lastKnownInstantFromClient =
ctx.queryParam(RemoteHoodieTableFileSystemView.LAST_INSTANT_TS, HoodieTimeline.INVALID_INSTANT_TS);
SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
- synchronized (view) {

Review comment:
I was mulling about the reloading of timeline that happens before the write lock.

```
@Override
public void sync() {
HoodieTimeline oldTimeline = getTimeline();
HoodieTimeline newTimeline = metaClient.reloadActiveTimeline().filterCompletedAndCompactionInstants();
try {
writeLock.lock();
runSync(oldTimeline, newTimeline);
} finally {
writeLock.unlock();
}
}
```

`runSync()` actually could init/reassign `metaClient`, so in theory removing synchornized could in theory make it non-serializable.


I would suggest that we either move the timeline reload into the write lock and leave this as-is. Whatever we change, we need to validate with more concurrent testing. So not sure if this is all worth the trouble.

Are you hitting real concurrency bottlenecks around this?

--
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-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org
;;;

---

08/Jul/21 06:57;vinoth;done. left a comment on the PR! 

 ;;;

---

08/Jul/21 06:58;githubbot;hudi-bot commented on pull request #3041:
URL: https://github.com/apache/hudi/pull/3041#issuecomment-876182983

## CI report:

* 2a77d2a41e46b630c523c00ed078e565e855d0a1 UNKNOWN


Bot commands
@hudi-bot supports the following commands:

- `@hudi-bot run travis` re-run the last Travis build
- `@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-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org
;;;

---

08/Jul/21 07:00;githubbot;hudi-bot edited a comment on pull request #3041:
URL: https://github.com/apache/hudi/pull/3041#issuecomment-876182983

## CI report:

* 2a77d2a41e46b630c523c00ed078e565e855d0a1 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=795)


Bot commands
@hudi-bot supports the following commands:

- `@hudi-bot run travis` re-run the last Travis build
- `@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-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org
;;;

---

08/Jul/21 07:48;githubbot;hudi-bot edited a comment on pull request #3041:
URL: https://github.com/apache/hudi/pull/3041#issuecomment-876182983

## CI report:

* 2a77d2a41e46b630c523c00ed078e565e855d0a1 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=795)


Bot commands
@hudi-bot supports the following commands:

- `@hudi-bot run travis` re-run the last Travis build
- `@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-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org
;;;

---

09/Jul/21 02:41;githubbot;chaplinthink commented on a change in pull request #3041:
URL: https://github.com/apache/hudi/pull/3041#discussion_r666633342

##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java
##########
@@ -132,15 +132,13 @@ private boolean syncIfLocalViewBehind(Context ctx) {
String lastKnownInstantFromClient =
ctx.queryParam(RemoteHoodieTableFileSystemView.LAST_INSTANT_TS, HoodieTimeline.INVALID_INSTANT_TS);
SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
- synchronized (view) {

Review comment:
Thanks a lot for reply. Do you mean the `synchornized ` is to ensure `HoodieTimeline newTimeline = metaClient.reloadActiveTimeline().filterCompletedAndCompactionInstants();` concurrently?

In fact, we are also to do this right ?

```
@Override
public void sync() {
HoodieTimeline oldTimeline = getTimeline();
try {
writeLock.lock();
HoodieTimeline newTimeline = metaClient.reloadActiveTimeline().filterCompletedAndCompactionInstants();
runSync(oldTimeline, newTimeline);
} finally {
writeLock.unlock();
}
}
```

I am confused when I see the code that we use `synchornized` and `writeLock` at the same time.

I agree to validate this with more concurrent testing. Currently i have not encountered concurrency bottlenecks.

--
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-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org
;;;

---

08/Aug/21 20:18;githubbot;hudi-bot edited a comment on pull request #3041:
URL: https://github.com/apache/hudi/pull/3041#issuecomment-876182983

## CI report:

* 2a77d2a41e46b630c523c00ed078e565e855d0a1 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=795)


Bot commands
The @flinkbot bot supports the following commands:

- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot 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-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org
;;;

---

09/Aug/21 04:22;githubbot;hudi-bot edited a comment on pull request #3041:
URL: https://github.com/apache/hudi/pull/3041#issuecomment-876182983

## CI report:

* 2a77d2a41e46b630c523c00ed078e565e855d0a1 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=795)


Bot commands
@hudi-bot supports the following commands:

- `@hudi-bot run travis` re-run the last Travis build
- `@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-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org
;;;

Start with hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java, especially syncIfLocalViewBehind and its synchronized(view) block. Compare this path with SyncableFileSystemView.sync(), including the reload and writeLock ordering, and review the discussion in pull request #3041. Done requires validating concurrent behavior before deciding whether the synchronization can be removed.

Written by the indexing model from the issue text.