[R4R] #476 write order id to file for large expire message by cryptom-dev · Pull Request #478 · bnb-chain/node

cryptom-dev · 2019-03-06T04:56:38Z

Make sure Jack or Erheng modify db timestamp column to persist correct date format

Make sure Zongjiang's ws trade timestamp changed from multiply 1000 to divide 10 ^ 6

Description

write order id to file for large expire message

Rationale

#476

wiki update : https://github.com/binance-chain/docs-internal/wiki/qa_runbook#publication-error

Example

will keep a file at /Users/zhaocong/.bnbchaind_publisher/data/essential/3087_ExecutionResults.log

Duplicated order id is because the trade-id on order is different (capture this log on non-breathe block)

height:3087
orders:
0:
D01E89F25E05DD00382DDAF4541E754B7DAEB9BE-1318
5:
E00476F64BA73E5FE3E2E7D20D1C64706B2F7915-1299
D01E89F25E05DD00382DDAF4541E754B7DAEB9BE-1318
E00476F64BA73E5FE3E2E7D20D1C64706B2F7915-1298
D01E89F25E05DD00382DDAF4541E754B7DAEB9BE-1318
E00476F64BA73E5FE3E2E7D20D1C64706B2F7915-1297
D01E89F25E05DD00382DDAF4541E754B7DAEB9BE-1318
D01E89F25E05DD00382DDAF4541E754B7DAEB9BE-1318
4:
E00476F64BA73E5FE3E2E7D20D1C64706B2F7915-1138

proposals:

3087_Accounts.log

height:3087
bnb1uqz8dajt5ul9lclzulfq68rywp4j77g46gn3q5
bnb16q0gnuj7qhwsqwpdmt69g8n4fd76awd7p8hgcu

Changes

dump large expire and send an empty msg keep qs is running
upgrade kafka sarama lib
change publish acknowledge level to local (we won't be blocked during upgrade kafka)

Preflight checks

build passed (make build)
tests passed (make test)
integration tests passed (make integration_test)
manual transaction test passed (cli invoke)

Already reviewed by

...

Related issues

#476

app/app.go

app/pub/publisher_kafka.go

unclezoro · 2019-03-06T06:25:04Z

app/pub/publisher_kafka.go

 		go pClient.UpdatePrometheusMetrics()
 	}

+	if _, err := os.Stat(publisher.essentialLogPath); os.IsNotExist(err) {


We get a function EnsureDir in github.com/tendermint/tendermint/libs/common, maybe we can reuse it.

app/pub/publisher_kafka.go

unclezoro · 2019-03-06T06:31:07Z

app/pub/publisher_kafka.go

-			Logger.Error("failed to publish", "topic", topic, "msg", avroMessage.String(), "err", err)
+			filePath := fmt.Sprintf("%s/%d_%s.log", publisher.essentialLogPath, height, tpe.String())
+			toWrite := []byte(avroMessage.EssentialMsg())
+			if err := ioutil.WriteFile(filePath, toWrite, 0644); err != nil {


can we skip if len(toWrite) is zero?

ioutil.WriteFile will open and close file each time, can we always hold a Writer?

ioutil.WriteFile will open and close file each time, can we always hold a Writer?

each file is different file here with height and message type in filename, and as I said we should hit this code path very rare (I hope once per quarter..)

oh. I See.

2.ext3文件系统下单个目录里的最大文件数无特别的限制，是受限于所在文件系统的inode数。我在RHEL5u5的ext3文件系统中测试，在一个目录下，touch了100万个文件是没有问题的。但是肯定会受到所在文件系统的inode数的限制。

Each block will create serval file, maybe reach the system limit in the long run.

Each block will create serval file, maybe reach the system limit in the long run.

we will delete once there is files for a block, this is devops rule

darren-liu · 2019-03-06T11:17:15Z

app/pub/msgs.go

+		if _, ok := stat[order.Status]; !ok {
+			stat[order.Status] = order.OrderId
+		} else {
+			stat[order.Status] += fmt.Sprintf("\n%s", order.OrderId)


continuous Sprintf may be slow. There should be other solution.

continuous Sprintf may be slow. There should be other solution.

use strings.Builder instead

darren-liu · 2019-03-06T11:18:28Z

app/pub/msgs.go

 }
+
+func (msg Transfers) EssentialMsg() string {
+	// deliberated not implemented


why transfer not implemented?

i would suggest we implement every message types, even trade. If not, then it would dump nothing anyway.

why transfer not implemented?

i would suggest we implement every message types, even trade. If not, then it would dump nothing anyway.

for transfer:

qs doesn't subscribe to its topic (risk control is relying on that)

risk control can recover from explorer indexed transfers (pull mode)

we don't have a unique representation of transfer like order-id (we didn't save txhash in message)

for trade:
the problem is same with above point 3, (trade id is only generated during publication, not persisted anywhere). If we keep qty, price, sid, bid for a trade, it would be too much, in this case maybe we should recover from local publisher?

I think whether make this PR a more generic solution is depends on how we use it. My initial understanding is we only need this to cope with expire block (no trades, huge amount of expired orders, rare happen)

If we also want cover large normal block publication failure, I still think have a full node enabled local publisher is safer.

you are fine with only dump orders/proposals/accounts as discussed offline

cryptom-dev · 2019-03-11T11:34:27Z

Gopkg.toml


 [[constraint]]
  name = "github.com/Shopify/sarama"
-  version = "1.17.0"


complained by @erhenglu that current kafka version 1.1.0 disconnect frequently. We want upgrade kafka to 2.1.0 in new QA and PROD

when will that happen? do we have a pause the site when we upgrade?

when will that happen? do we have a pause the site when we upgrade?

rolling upgrade kafka won't pause the site (but due to leader re-election, there will be some 4-6s height delay during rolling bounce kafka as I tested in QA)

darren-liu · 2019-03-13T01:54:14Z

Gopkg.toml


 [[constraint]]
  name = "github.com/Shopify/sarama"
-  version = "1.17.0"


when will that happen? do we have a pause the site when we upgrade?

darren-liu · 2019-03-13T04:46:31Z

app/pub/msgs.go

+// 3. we don't have a unique representation of transfer like order-id (we didn't save txhash in message)
+//
+// for trade:
+// the problem is same with above point 3, (trade id is only generated during publication, not persisted anywhere).


can I confirm the trade id is deterministic across all Witness publisher?

darren-liu · 2019-03-15T09:39:59Z

app/pub/publisher_kafka.go

+		kafkaMsg := publisher.prepareMessage(topic, strconv.FormatInt(height, 10), timestamp, tpe, msg)
+		if partition, offset, err := publisher.publishWithRetry(kafkaMsg, topic); err == nil {
+			Logger.Info("published", "topic", topic, "msg", avroMessage.String(), "offset", offset, "partition", partition)
+			if essMsg, ok := avroMessage.(EssMsg); ok {


please drop this.

please drop this.

thx..

please drop this.

fixed

cryptom-dev · 2019-03-18T07:52:57Z

app/app.go

 		pub.IsLive {
 		if height >= app.publicationConfig.FromHeightInclusive {
-			app.publish(tradesToPublish, &proposals, blockFee, ctx, height, blockTime.Unix())
+			app.publish(tradesToPublish, &proposals, blockFee, ctx, height, blockTime.UnixNano())


need confirm down streams are good

need confirm down streams are good

@erhenglu want next time upgrade, need a pause on qs to change DB schema

cryptom-dev self-assigned this Mar 6, 2019

cryptom-dev requested review from darren-liu, rickyyangz and unclezoro March 6, 2019 04:56

cryptom-dev changed the title ~~#476 write order id to file for large expire message~~ [R4R] #476 write order id to file for large expire message Mar 6, 2019

cryptom-dev commented Mar 6, 2019

View reviewed changes

app/app.go Outdated Show resolved Hide resolved

cryptom-dev changed the base branch from master to develop March 6, 2019 05:11

unclezoro reviewed Mar 6, 2019

View reviewed changes

app/pub/publisher_kafka.go Show resolved Hide resolved

unclezoro reviewed Mar 6, 2019

View reviewed changes

app/pub/publisher_kafka.go Outdated Show resolved Hide resolved

unclezoro reviewed Mar 6, 2019

View reviewed changes

cryptom-dev force-pushed the largeexpire branch from bc8ccee to 7d74fee Compare March 6, 2019 07:50

darren-liu suggested changes Mar 6, 2019

View reviewed changes

cryptom-dev force-pushed the largeexpire branch 5 times, most recently from af80d98 to 0af58a9 Compare March 11, 2019 11:30

cryptom-dev commented Mar 11, 2019

View reviewed changes

cryptom-dev force-pushed the largeexpire branch from 0af58a9 to f99de0b Compare March 13, 2019 02:21

darren-liu suggested changes Mar 15, 2019

View reviewed changes

cryptom-dev force-pushed the largeexpire branch from f99de0b to 16f2737 Compare March 15, 2019 11:01

cryptom-dev commented Mar 18, 2019

View reviewed changes

cryptom-dev force-pushed the largeexpire branch from 16f2737 to 0ef3c13 Compare March 19, 2019 01:57

unclezoro approved these changes Mar 20, 2019

View reviewed changes

cryptom-dev force-pushed the largeexpire branch from 0ef3c13 to fa3254b Compare March 20, 2019 06:51

darren-liu approved these changes Mar 20, 2019

View reviewed changes

cryptom-dev changed the title ~~[R4R] #476 write order id to file for large expire message~~ [DO NOT MERGE] #476 write order id to file for large expire message Mar 20, 2019

cryptom-dev force-pushed the largeexpire branch from fa3254b to bd6a17f Compare March 20, 2019 08:42

#476 write order id to file for large expire message

61acc7d

cryptom-dev force-pushed the largeexpire branch from bd6a17f to 61acc7d Compare March 21, 2019 02:41

cryptom-dev changed the title ~~[DO NOT MERGE] #476 write order id to file for large expire message~~ [R4R] #476 write order id to file for large expire message Mar 29, 2019

cryptom-dev merged commit cd0bd3c into develop Mar 29, 2019

unclezoro deleted the largeexpire branch May 10, 2022 06:09

Conversation

cryptom-dev commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Make sure Jack or Erheng modify db timestamp column to persist correct date format

Make sure Zongjiang's ws trade timestamp changed from multiply 1000 to divide 10 ^ 6

Description

Rationale

Example

Changes

Preflight checks

Already reviewed by

Related issues

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unclezoro Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cryptom-dev commented Mar 6, 2019 •

edited

Loading

unclezoro Mar 6, 2019 •

edited

Loading