Skip to content

Conversation

@alexkli
Copy link
Contributor

@alexkli alexkli commented Apr 18, 2020

Follow up from #45 and #32. We can save some time (~800ms in my tests) by not making a request to /api/v1/api-docs to detect whether the openwhisk system supports concurrency or not. This check doesn't always work and we handle it later gracefully since #32 anyway.

Note: this is on top of PR #56, see here for the actual changeset: console-spinner...optimistic-concurrency

@alexkli alexkli added this to the 1.3 milestone Apr 18, 2020
@alexkli alexkli changed the title Drop concurrency api check for performance drop concurrency api check for performance Apr 18, 2020
@alexkli alexkli requested review from dgrove-oss and rabbah April 18, 2020 07:50
@rabbah
Copy link
Member

rabbah commented Apr 18, 2020

Needs a rebase.

@alexkli
Copy link
Contributor Author

alexkli commented Apr 20, 2020

@rabbah Thanks for the reviews!

Regarding the conflict: that would be entirely avoidable if (true) merging would be allowed on PRs: this PR was simply on top of #56 and if that one would have been merged instead of rebased, git would know and this here would not show as a conflict.

However, the repo settings here prevent one from choosing merge as an option. I assume that's an Openwhisk project policy? Could it be allowed?

I agree with preferring clean histories without a lot of "merge" commits on master. However, in some cases like this it's not avoidable and one is loosing a major git feature if merging is entirely prevented. In our in-house development we address this by allowing all options on PRs, but ask people to avoid merging unless necessary. This way you still avoid the majority of merge commits on master.

I guess I should bring this to the dev list?

/cc @dgrove-oss

@rabbah
Copy link
Member

rabbah commented Apr 20, 2020

The project uses linear github histories and we avoid merges vs rebase/squash and merge.
The merge policy has been discussed in the past on the dev list I think - I'd rather we don't rehash it but of course you're welcome to.

We can do rebase and merge as oppose to squash and merge if you indicate that in the PR. Otherwise the default is squash and merge.

@alexkli
Copy link
Contributor Author

alexkli commented Apr 20, 2020

Sure, but that policy leads to more work in case one has concurrent branches like here :-)

Neither rebase nor squash would have helped here, only a proper merge. It seems not an efficient use of git to generally prevent that.

@alexkli alexkli merged commit d050f9d into master Apr 22, 2020
@alexkli alexkli deleted the optimistic-concurrency branch April 22, 2020 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants